Repository: canonical/operator
Pull request #1036 (2023-10-09 23:58:19)

ci: Pass LIBJUJU_VERSION_SPECIFIER to the mysql-k8s-operator unit tests
In canonical/mysql-k8s-operator#313, the unit tests were changed to expect a LIBJUJU_VERSION_SPECIFIER envvar to be present when the tests are run. This is used in those tests to run the tests simulating Juju 2.9 and 3.1.

Adjust our running of the mysql-k8s-operator tests to also provide this value, fixed at 3.1. (This is also present for the other 3 DB operators, but that should do no harm).

tonyandrewmeyer: @benhoyt a couple of alternatives:

* We pass through `2.9` as the value. That seems better in terms of "always check you run with the oldest supported version", but worse in terms of "good to exercise that we haven't broken things for their use of secrets" (which seems to be the main thing they're using the version check for at the moment).
* We add in the matrix of values in our CI as well, testing both `2.9` and `3.1` values. This would avoid picking one, but would also mean that we test the other three DB charms twice for no reason, unless this was spun out more, or unless you can do a "sub matrix", which I don't think is possible in GitHub actions.
tonyandrewmeyer: > * We add in the matrix of values in our CI as well, testing both `2.9` and `3.1` values. This would avoid picking one, but would also mean that we test the other three DB charms twice for no reason, unless this was spun out more, or unless you can do a "sub matrix", which I don't think is possible in GitHub actions.

Hmm, maybe [include](https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs#expanding-or-adding-matrix-configurations) makes this possible? I haven't used that before. Should I try with that instead?
benhoyt: This is a reasonable fix, but per discussion, it's definitely not ideal that the mysql-k8s-operator's *unit* test now require a certain environment variable to be set. We should be able to check out a charm repo and run `tox -e unit` without any further ado.

So let's put up a PR against their charm to fix that. They probably shouldn't be depending on that environment variable at all, but just have a single `has_secrets` fixture that patches `JujuVersion.has_secrets` with True.

Separately, it's a bit strange that default for `JujuVersion.from_environ` is 0.0.0. I wonder if we should override that to "latest stable version of Juju", at least in ops.testing?
tonyandrewmeyer: > So let's put up a PR against their charm to fix that. They probably shouldn't be depending on that environment variable at all, but just have a single `has_secrets` fixture that patches `JujuVersion.has_secrets` with True.

Opened [canonical/mysql-k8s-operator:#316](https://github.com/canonical/mysql-k8s-operator/pull/316). I left the `without_juju_secrets` fixture in place because it does seem like being explicit there is worthwhile, and if we do change the default JujuVersion value in testing then we'd avoid breakage (at least for this one charm).

 > Separately, it's a bit strange that default for `JujuVersion.from_environ` is 0.0.0. I wonder if we should override that to "latest stable version of Juju", at least in ops.testing?

Opened #1037.
jameinel: I do think you want coverage of mysql pre and post, because how it shares
passwords with the world changes (juju 2.9 doesn't have secrets as a way to
share the password so they have to put it in relation data, vs 3.1)

I definitely don't like an ENV var for unit testing (at the least it means
when running the unit test you aren't testing  both code paths in one suite
run)

Its plausible that Harness should make it easier to express the behaviors
that you want.

On Tue, Oct 10, 2023, 00:21 Tony Meyer ***@***.***> wrote:

> So let's put up a PR against their charm to fix that. They probably
> shouldn't be depending on that environment variable at all, but just have a
> single has_secrets fixture that patches JujuVersion.has_secrets with True.
>
> Opened canonical/mysql-k8s-operator:#316
> <https://github.com/canonical/mysql-k8s-operator/pull/316>. I left the
> without_juju_secrets fixture in place because it does seem like being
> explicit there is worthwhile, and if we do change the default JujuVersion
> value in testing then we'd avoid breakage (at least for this one charm).
>
> Separately, it's a bit strange that default for JujuVersion.from_environ
> is 0.0.0. I wonder if we should override that to "latest stable version of
> Juju", at least in ops.testing?
>
> Opened #1037 <https://github.com/canonical/operator/issues/1037>.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/canonical/operator/pull/1036#issuecomment-1754332780>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABRQ7O2S3NEG7APZZCBWWTX6TENJAVCNFSM6AAAAAA5ZPQBHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJUGMZTENZYGA>
> .
> You are receiving this because you are subscribed to this thread.Message
> ID: ***@***.***>
>

jameinel: I looked over the mysql PR, and it really seems like we want a clean way to
fake the juju version that we are testing against, not patch out a method
on that object.

John
=:->

On Tue, Oct 10, 2023, 07:31 John A Meinel ***@***.***> wrote:

> I do think you want coverage of mysql pre and post, because how it shares
> passwords with the world changes (juju 2.9 doesn't have secrets as a way to
> share the password so they have to put it in relation data, vs 3.1)
>
> I definitely don't like an ENV var for unit testing (at the least it means
> when running the unit test you aren't testing  both code paths in one suite
> run)
>
> Its plausible that Harness should make it easier to express the behaviors
> that you want.
>
> On Tue, Oct 10, 2023, 00:21 Tony Meyer ***@***.***> wrote:
>
>> So let's put up a PR against their charm to fix that. They probably
>> shouldn't be depending on that environment variable at all, but just have a
>> single has_secrets fixture that patches JujuVersion.has_secrets with
>> True.
>>
>> Opened canonical/mysql-k8s-operator:#316
>> <https://github.com/canonical/mysql-k8s-operator/pull/316>. I left the
>> without_juju_secrets fixture in place because it does seem like being
>> explicit there is worthwhile, and if we do change the default JujuVersion
>> value in testing then we'd avoid breakage (at least for this one charm).
>>
>> Separately, it's a bit strange that default for JujuVersion.from_environ
>> is 0.0.0. I wonder if we should override that to "latest stable version of
>> Juju", at least in ops.testing?
>>
>> Opened #1037 <https://github.com/canonical/operator/issues/1037>.
>>
>> —
>> Reply to this email directly, view it on GitHub
>> <https://github.com/canonical/operator/pull/1036#issuecomment-1754332780>,
>> or unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/AABRQ7O2S3NEG7APZZCBWWTX6TENJAVCNFSM6AAAAAA5ZPQBHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJUGMZTENZYGA>
>> .
>> You are receiving this because you are subscribed to this thread.Message
>> ID: ***@***.***>
>>
>

benhoyt: Thanks @jameinel -- agreed. I'm going to merge this to get our CI working in the meantime (with a TODO comment saying to remove it once https://github.com/canonical/mysql-k8s-operator/pull/316 is fixed), and we can continue the discussion of improving this at #1037.

Patch:
diff --git a/ops/charm.py b/ops/charm.py
index e2b39fa..77a1f0b 100755
--- a/ops/charm.py
+++ b/ops/charm.py
@@ -52,6 +52,7 @@ if TYPE_CHECKING:
         '_RelationMetaDict', {
             'interface': Required[str],
             'limit': int,
+            'optional': bool,
             'scope': _Scopes},
         total=False)
 
@@ -1300,6 +1301,14 @@ class RelationMeta:
     Will be either ``"global"`` or ``"container"``.
     """
 
+    optional: bool
+    """If True, the relation is considered optional.
+
+    This value is informational only and is not used by Juju itself (all
+    relations are optional from Juju's perspective), but it may be set in
+    ``metadata.yaml`` and used by the charm code if appropriate.
+    """
+
     VALID_SCOPES = ['global', 'container']
 
     def __init__(self, role: RelationRole, relation_name: str, raw: '_RelationMetaDict'):
@@ -1319,6 +1328,8 @@ class RelationMeta:
             raise TypeError("scope should be one of {}; not '{}'".format(
                 ', '.join(f"'{s}'" for s in self.VALID_SCOPES), self.scope))
 
+        self.optional = raw.get('optional', False)
+
 
 class StorageMeta:
     """Object containing metadata about a storage definition."""


Test patch:
diff --git a/.github/workflows/db-charm-tests.yaml b/.github/workflows/db-charm-tests.yaml
index 46cef24..2877f14 100644
--- a/.github/workflows/db-charm-tests.yaml
+++ b/.github/workflows/db-charm-tests.yaml
@@ -43,3 +43,8 @@ jobs:
 
       - name: Run the charm's unit tests
         run: tox -vve unit
+        # TODO: remove this once https://github.com/canonical/mysql-k8s-operator/pull/316 is fixed
+        env:
+          # This env var is only to indicate Juju version to "simulate" in the mysqk-k8s-operator
+          # unit tests. No libjuju is being actually used in unit testing.
+          LIBJUJU_VERSION_SPECIFIER: 3.1
diff --git a/test/test_charm.py b/test/test_charm.py
index e0b748c..e2fc510 100755
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -362,7 +362,6 @@ containers:
         self.assertEqual(charm.count, 2)
 
     def test_relations_meta(self):
-
         # language=YAML
         self.meta = ops.CharmMeta.from_yaml('''
 name: my-charm
@@ -373,15 +372,18 @@ requires:
     scope: container
   metrics:
     interface: prometheus-scraping
+    optional: true
 ''')
 
         self.assertEqual(self.meta.requires['database'].interface_name, 'mongodb')
         self.assertEqual(self.meta.requires['database'].limit, 1)
         self.assertEqual(self.meta.requires['database'].scope, 'container')
+        self.assertFalse(self.meta.requires['database'].optional)
 
         self.assertEqual(self.meta.requires['metrics'].interface_name, 'prometheus-scraping')
         self.assertIsNone(self.meta.requires['metrics'].limit)
         self.assertEqual(self.meta.requires['metrics'].scope, 'global')  # Default value
+        self.assertTrue(self.meta.requires['metrics'].optional)
 
     def test_relations_meta_limit_type_validation(self):
         with self.assertRaisesRegex(TypeError, "limit should be an int, not <class 'str'>"):