Repository: canonical/operator
Pull request #318 (2020-06-02 14:07:37)

Simplified the CharmBase init signature.
We need to talk about deprecating/changing behaviour before we land this.

jameinel: The problem is that we break compatibility in 2 ways:

a) main.main() wants to stop initializing the charm_cls it has with 2
parameters
b) CharmBase wants to stop accepting 2 parameters.

The problem is that *user* code sits between 2 places that are Operator
code. The User code still needs to pass through _marker, so it hasn't
actually deprecated us not *passing* it _marker.


On Wed, Jun 3, 2020 at 12:21 PM chipaca <notifications@github.com> wrote:

> *@chipaca* commented on this pull request.
>
> If I understand correctly, while we want this to be the end game, it'll
> break people so instead we'll change things so that it's got key=_marker,
> and then raise (and log) a DeprecationWarning if key is set to anything
> other than _marker?
> If so I think that's reasonable to have in 0.7, and 0.8 can drop the
> warning and the argument entirely.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/canonical/operator/pull/318#pullrequestreview-423324453>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABRQ7JMVAD4B23LXCIJRULRUYBXJANCNFSM4NQYMQ4Q>
> .
>


Patch:
diff --git a/CODE_OF_CONDUCT.md b/CODE_OF_CONDUCT.md
new file mode 100644
index 0000000..b5bc1b1
--- /dev/null
+++ b/CODE_OF_CONDUCT.md
@@ -0,0 +1,78 @@
+# Contributor Covenant Code of Conduct
+
+## Our Pledge
+
+In the interest of fostering an open and welcoming environment, we as
+contributors and maintainers pledge to making participation in our project and
+our community a harassment-free experience for everyone, regardless of age, body
+size, disability, ethnicity, sex characteristics, gender identity and expression,
+level of experience, education, socio-economic status, nationality, personal
+appearance, race, religion, or sexual identity and orientation.
+
+## Our Standards
+
+Examples of behavior that contributes to creating a positive environment
+include:
+
+* Using welcoming and inclusive language
+* Being respectful of differing viewpoints and experiences
+* Gracefully accepting constructive criticism
+* Focusing on what is best for the community
+* Showing empathy towards other community members
+
+Examples of unacceptable behavior by participants include:
+
+* The use of sexualized language or imagery and unwelcome sexual attention or
+ advances
+* Trolling, insulting/derogatory comments, and personal or political attacks
+* Public or private harassment
+* Publishing others' private information, such as a physical or electronic
+ address, without explicit permission
+* Other conduct which could reasonably be considered inappropriate in a
+ professional setting
+
+## Our Responsibilities
+
+Project maintainers are responsible for clarifying the standards of acceptable
+behavior and are expected to take appropriate and fair corrective action in
+response to any instances of unacceptable behavior.
+
+Project maintainers have the right and responsibility to remove, edit, or
+reject comments, commits, code, wiki edits, issues, and other contributions
+that are not aligned to this Code of Conduct, or to ban temporarily or
+permanently any contributor for other behaviors that they deem inappropriate,
+threatening, offensive, or harmful.
+
+## Scope
+
+This Code of Conduct applies both within project spaces and in public spaces
+when an individual is representing the project or its community. Examples of
+representing a project or community include using an official project e-mail
+address, posting via an official social media account, or acting as an appointed
+representative at an online or offline event. Representation of a project may be
+further defined and clarified by project maintainers.
+
+## Enforcement
+
+Instances of abusive, harassing, or otherwise unacceptable behavior
+may be reported by contacting the project team at
+charmcrafters@lists.launchpad.net. All complaints will be reviewed and
+investigated and will result in a response that is deemed necessary
+and appropriate to the circumstances. The project team is obligated to
+maintain confidentiality with regard to the reporter of an incident.
+Further details of specific enforcement policies may be posted
+separately.
+
+Project maintainers who do not follow or enforce the Code of Conduct in good
+faith may face temporary or permanent repercussions as determined by other
+members of the project's leadership.
+
+## Attribution
+
+This Code of Conduct is adapted from the [Contributor Covenant][homepage], version 1.4,
+available at https://www.contributor-covenant.org/version/1/4/code-of-conduct.html
+
+[homepage]: https://www.contributor-covenant.org
+
+For answers to common questions about this code of conduct, see
+https://www.contributor-covenant.org/faq
diff --git a/ops/__init__.py b/ops/__init__.py
index e3207f5..f17b296 100644
--- a/ops/__init__.py
+++ b/ops/__init__.py
@@ -14,7 +14,7 @@
 
 """The Operator Framework."""
 
-__version__ = '0.6.1'
+from .version import version as __version__  # noqa: F401 (imported but unused)
 
 # Import here the bare minimum to break the circular import between modules
-from . import charm  # NOQA
+from . import charm  # noqa: F401 (imported but unused)
diff --git a/ops/charm.py b/ops/charm.py
index 3d98b6e..4f6ecc6 100755
--- a/ops/charm.py
+++ b/ops/charm.py
@@ -335,18 +335,13 @@ class CharmBase(Object):
     Args:
         framework: The framework responsible for managing the Model and events for this
             Charm.
-        key: Arbitrary key to distinguish this instance of CharmBase from another.
-            Generally is None when initialized by the framework. For charms instantiated by
-            main.main(), this is currenly None.
+        key: Ignored; will remove after deprecation period of the signature change.
     """
 
     on = CharmEvents()
 
-    def __init__(self, framework: Framework, key: typing.Optional[str]):
-        """Initialize the Charm with its framework and application name.
-
-        """
-        super().__init__(framework, key)
+    def __init__(self, framework: Framework, key: typing.Optional = None):
+        super().__init__(framework, None)
 
         for relation_name in self.framework.meta.relations:
             relation_name = relation_name.replace('-', '_')
diff --git a/ops/main.py b/ops/main.py
index c387271..77fa536 100755
--- a/ops/main.py
+++ b/ops/main.py
@@ -1,5 +1,4 @@
-#!/usr/bin/env python3
-# Copyright 2019 Canonical Ltd.
+# Copyright 2019-2020 Canonical Ltd.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -13,9 +12,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import inspect
 import os
 import subprocess
 import sys
+import warnings
 from pathlib import Path
 
 import yaml
@@ -299,7 +300,17 @@ def main(charm_class):
     charm_state_path = charm_dir / CHARM_STATE_FILE
     framework = ops.framework.Framework(charm_state_path, charm_dir, meta, model)
     try:
-        charm = charm_class(framework, None)
+        sig = inspect.signature(charm_class)
+        try:
+            sig.bind(framework)
+        except TypeError:
+            msg = (
+                "the second argument, 'key', has been deprecated and will be "
+                "removed after the 0.7 release")
+            warnings.warn(msg, DeprecationWarning)
+            charm = charm_class(framework, None)
+        else:
+            charm = charm_class(framework)
         dispatcher.ensure_event_links(charm)
 
         # TODO: Remove the collect_metrics check below as soon as the relevant
diff --git a/ops/version.py b/ops/version.py
new file mode 100644
index 0000000..a285947
--- /dev/null
+++ b/ops/version.py
@@ -0,0 +1,50 @@
+# Copyright 2020 Canonical Ltd.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import subprocess
+from pathlib import Path
+
+__all__ = ('version',)
+
+_FALLBACK = '0.7'  # this gets bumped after release
+
+
+def _get_version():
+    version = _FALLBACK + ".dev0+unknown"
+
+    p = Path(__file__).parent
+    if (p.parent / '.git').exists():
+        try:
+            proc = subprocess.run(
+                ['git', 'describe', '--tags', '--long', '--dirty'],
+                stdout=subprocess.PIPE,
+                stderr=subprocess.DEVNULL,
+                cwd=p,
+                check=True)
+        except Exception:
+            pass
+        else:
+            version = proc.stdout.strip().decode('utf8')
+            if '-' in version:
+                # version will look like <tag>-<#commits>-g<hex>[-dirty]
+                # in terms of PEP 440, the tag we'll make sure is a 'public version identifier';
+                # everything after the first - needs to be a 'local version'
+                public, local = version.split('-', 1)
+                version = public + '+' + local.replace('-', '.')
+                # version now <tag>+<#commits>.g<hex>[.dirty]
+                # which is PEP440-compliant (as long as <tag> is :-)
+    return version
+
+
+version = _get_version()
diff --git a/requirements.txt b/requirements.txt
index e4a3024..5500f00 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,1 +1,1 @@
--e . #  i.e. get them from setup.py
+PyYAML
diff --git a/setup.py b/setup.py
index 6ac0c0b..ceb8668 100644
--- a/setup.py
+++ b/setup.py
@@ -12,7 +12,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import ast
+from importlib.util import spec_from_file_location, module_from_spec
+from pathlib import Path
 from setuptools import setup, find_packages
 
 
@@ -23,49 +24,52 @@ def _read_me() -> str:
 
 
 def _get_version() -> str:
-    # ops.__init__ needs to pull in ops.charm to work around a circular
-    # import so we can't import it here as that pulls in yaml which isn't
-    # necessarily there yet.
-    version = 'unknown'
-    with open("ops/__init__.py", "rt", encoding="utf8") as fh:
-        source = fh.read()
-    code = ast.parse(source)
-    for node in code.body:
-        if isinstance(node, ast.Assign):
-            targets = [i.id for i in node.targets]
-            if '__version__' in targets:
-                if isinstance(node.value, ast.Str):
-                    # Python < 3.8
-                    version = node.value.s
-                else:
-                    version = node.value.value
-                break
-    return version
+    """Get the version via ops/version.py, without loading ops/__init__.py"""
+    spec = spec_from_file_location('ops.version', 'ops/version.py')
+    module = module_from_spec(spec)
+    spec.loader.exec_module(module)
 
+    return module.version
 
-setup(
-    name="ops",
-    version=_get_version(),
-    description="The Python library behind great charms",
-    long_description=_read_me(),
-    long_description_content_type="text/markdown",
-    license="Apache-2.0",
-    url="https://github.com/canonical/operator",
-    author="The Charmcraft team at Canonical Ltd.",
-    author_email="charmcraft@lists.launchpad.net",
-    packages=find_packages(include=('ops', 'ops.*')),
-    classifiers=[
-        "Programming Language :: Python :: 3",
-        "License :: OSI Approved :: Apache Software License",
-        "Development Status :: 4 - Beta",
 
-        "Intended Audience :: Developers",
-        "Intended Audience :: System Administrators",
-        "Operating System :: MacOS :: MacOS X",
-        "Operating System :: POSIX :: Linux",
-        # include Windows once we're running tests there also
-        # "Operating System :: Microsoft :: Windows",
-    ],
-    python_requires='>=3.5',
-    install_requires=["PyYAML"],
-)
+version = _get_version()
+version_path = Path("ops/version.py")
+version_backup = Path("ops/version.py~")
+version_path.rename(version_backup)
+try:
+    with version_path.open("wt", encoding="utf8") as fh:
+        fh.write('''\
+# this is a generated file
+
+version = {!r}
+'''.format(version))
+
+    setup(
+        name="ops",
+        version=version,
+        description="The Python library behind great charms",
+        long_description=_read_me(),
+        long_description_content_type="text/markdown",
+        license="Apache-2.0",
+        url="https://github.com/canonical/operator",
+        author="The Charmcraft team at Canonical Ltd.",
+        author_email="charmcraft@lists.launchpad.net",
+        packages=find_packages(include=('ops', 'ops.*')),
+        classifiers=[
+            "Programming Language :: Python :: 3",
+            "License :: OSI Approved :: Apache Software License",
+            "Development Status :: 4 - Beta",
+
+            "Intended Audience :: Developers",
+            "Intended Audience :: System Administrators",
+            "Operating System :: MacOS :: MacOS X",
+            "Operating System :: POSIX :: Linux",
+            # include Windows once we're running tests there also
+            # "Operating System :: Microsoft :: Windows",
+        ],
+        python_requires='>=3.5',
+        install_requires=["PyYAML"],
+    )
+
+finally:
+    version_backup.rename(version_path)


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index 4500439..0d1c0c0 100644
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -132,7 +132,7 @@ class Harness:
         # Note: jam 2020-03-01 This is so that errors in testing say MyCharm has no attribute foo,
         # rather than TestCharm has no attribute foo.
         TestCharm.__name__ = self._charm_cls.__name__
-        self._charm = TestCharm(self._framework, self._framework.meta.name)
+        self._charm = TestCharm(self._framework)
 
     def _create_meta(self, charm_metadata, action_metadata):
         """Create a CharmMeta object.
diff --git a/test/test_charm.py b/test/test_charm.py
index be615ff..41bbccb 100755
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -1,5 +1,4 @@
-#!/usr/bin/python3
-# Copyright 2019 Canonical Ltd.
+# Copyright 2019-2020 Canonical Ltd.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -84,7 +83,7 @@ class TestCharm(unittest.TestCase):
         self.assertIn('custom', events)
 
         framework = self.create_framework()
-        charm = MyCharm(framework, None)
+        charm = MyCharm(framework)
         charm.on.start.emit()
 
         self.assertEqual(charm.started, True)
@@ -98,7 +97,7 @@ class TestCharm(unittest.TestCase):
         class MyCharm(CharmBase):
             pass
 
-        charm = MyCharm(framework, None)
+        charm = MyCharm(framework)
         self.assertEqual(charm.app, framework.model.app)
         self.assertEqual(charm.unit, framework.model.unit)
         self.assertEqual(charm.meta, framework.meta)
@@ -142,7 +141,7 @@ peers:
    interface: peer2
 ''')
 
-        charm = MyCharm(self.create_framework(), None)
+        charm = MyCharm(self.create_framework())
 
         rel = charm.framework.model.get_relation('req1', 1)
         unit = charm.framework.model.get_unit('remote/0')
@@ -212,7 +211,7 @@ storage:
         self.assertEqual(self.meta.storages['stor3'].multiple_range, (2, None))
         self.assertEqual(self.meta.storages['stor-4'].multiple_range, (2, 4))
 
-        charm = MyCharm(self.create_framework(), None)
+        charm = MyCharm(self.create_framework())
 
         charm.on['stor1'].storage_attached.emit()
         charm.on['stor2'].storage_detaching.emit()
@@ -274,7 +273,7 @@ start:
 
         os.environ['JUJU_{}_NAME'.format(cmd_type.upper())] = 'foo-bar'
         framework = self.create_framework()
-        charm = MyCharm(framework, None)
+        charm = MyCharm(framework)
 
         events = list(MyCharm.on.events())
         self.assertIn('foo_bar_action', events)
@@ -313,14 +312,10 @@ start:
 
         os.environ['JUJU_{}_NAME'.format(cmd_type.upper())] = 'start'
         framework = self.create_framework()
-        charm = MyCharm(framework, None)
+        charm = MyCharm(framework)
 
         with self.assertRaises(RuntimeError):
             charm.on.start_action.emit()
 
     def test_action_event_defer_fails(self):
         self._test_action_event_defer_fails('action')
-
-
-if __name__ == "__main__":
-    unittest.main()
diff --git a/test/test_main.py b/test/test_main.py
index f1eb3d5..ae8b9be 100755
--- a/test/test_main.py
+++ b/test/test_main.py
@@ -1,5 +1,4 @@
-#!/usr/bin/env python3
-# Copyright 2019 Canonical Ltd.
+# Copyright 2019-2020 Canonical Ltd.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -24,7 +23,9 @@ import sys
 import tempfile
 import unittest
 import importlib.util
+import warnings
 from pathlib import Path
+from unittest.mock import patch
 
 from ops.charm import (
     CharmBase,
@@ -45,6 +46,7 @@ from ops.charm import (
     ActionEvent,
     CollectMetricsEvent,
 )
+from ops.main import main
 
 from .test_helpers import fake_script, fake_script_calls
 
@@ -73,6 +75,62 @@ class EventSpec:
         self.model_name = model_name
 
 
+class CharmInitTestCase(unittest.TestCase):
+
+    def _check(self, charm_class):
+        """Helper for below tests."""
+        fake_environ = {
+            'JUJU_UNIT_NAME': 'test_main/0',
+            'JUJU_MODEL_NAME': 'mymodel',
+        }
+        with patch.dict(os.environ, fake_environ):
+            with patch('ops.main._emit_charm_event'):
+                with patch('ops.main._get_charm_dir') as mock_charmdir:
+                    with tempfile.TemporaryDirectory() as tmpdirname:
+                        tmpdirname = Path(tmpdirname)
+                        fake_metadata = tmpdirname / 'metadata.yaml'
+                        with fake_metadata.open('wb') as fh:
+                            fh.write(b'name: test')
+                        mock_charmdir.return_value = tmpdirname
+
+                        with warnings.catch_warnings(record=True) as warnings_cm:
+                            main(charm_class)
+
+        return warnings_cm
+
+    def test_init_signature_passthrough(self):
+        class MyCharm(CharmBase):
+
+            def __init__(self, *args):
+                super().__init__(*args)
+
+        warn_cm = self._check(MyCharm)
+        self.assertFalse(warn_cm)
+
+    def test_init_signature_both_arguments(self):
+        class MyCharm(CharmBase):
+
+            def __init__(self, framework, somekey):
+                super().__init__(framework, somekey)
+
+        warn_cm = self._check(MyCharm)
+        self.assertEqual(len(warn_cm), 1)
+        (warn,) = warn_cm
+        self.assertTrue(issubclass(warn.category, DeprecationWarning))
+        self.assertEqual(str(warn.message), (
+            "the second argument, 'key', has been deprecated and will be removed "
+            "after the 0.7 release"))
+
+    def test_init_signature_only_framework(self):
+        class MyCharm(CharmBase):
+
+            def __init__(self, framework):
+                super().__init__(framework)
+
+        warn_cm = self._check(MyCharm)
+        self.assertFalse(warn_cm)
+
+
 class TestMain(abc.ABC):
 
     @abc.abstractmethod
@@ -547,6 +605,16 @@ log_debug: {}
         self.assertEqual(state['status_name'], 'blocked')
         self.assertEqual(state['status_message'], 'help meeee')
 
+    def test_foo(self):
+        # base64 encoding is used to avoid null bytes.
+        charm_config = base64.b64encode(pickle.dumps({
+            'STATE_FILE': self._state_file,
+        }))
+
+        # First run "install" to make sure all hooks are set up.
+        state = self._simulate_event(EventSpec(InstallEvent, 'install', charm_config=charm_config))
+        self.assertEqual(state['observed_event_types'], [InstallEvent])
+
 
 class TestMainWithNoDispatch(TestMain, unittest.TestCase):
     has_dispatch = False
@@ -801,7 +869,3 @@ class TestMainWithDispatchAsScript(TestMainWithDispatch):
         dispatch = self.JUJU_CHARM_DIR / 'dispatch'
         subprocess.check_call([str(dispatch)],
                               env=env, cwd=str(self.JUJU_CHARM_DIR))
-
-
-if __name__ == "__main__":
-    unittest.main()
diff --git a/test/test_testing.py b/test/test_testing.py
index b07b6d5..4b273f7 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -1,5 +1,4 @@
-#!/usr/bin/python3
-# Copyright 2019 Canonical Ltd.
+# Copyright 2019-2020 Canonical Ltd.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -96,8 +95,8 @@ class TestHarness(unittest.TestCase):
         class InitialDataTester(CharmBase):
             """Record the relation-changed events."""
 
-            def __init__(self, framework, charm_name):
-                super().__init__(framework, charm_name)
+            def __init__(self, framework):
+                super().__init__(framework)
                 self.observed_events = []
                 self.framework.observe(self.on.db_relation_changed, self._on_db_relation_changed)
 
@@ -145,8 +144,8 @@ class TestHarness(unittest.TestCase):
         class InitialDataTester(CharmBase):
             """Record the relation-changed events."""
 
-            def __init__(self, framework, charm_name):
-                super().__init__(framework, charm_name)
+            def __init__(self, framework):
+                super().__init__(framework)
                 self.observed_events = []
                 self.framework.observe(self.on.cluster_relation_changed,
                                        self._on_cluster_relation_changed)
@@ -725,8 +724,8 @@ class RelationChangedViewer(Object):
 class RecordingCharm(CharmBase):
     """Record the events that we see, and any associated data."""
 
-    def __init__(self, framework, charm_name):
-        super().__init__(framework, charm_name)
+    def __init__(self, framework):
+        super().__init__(framework)
         self.changes = []
         self.framework.observe(self.on.config_changed, self.on_config_changed)
         self.framework.observe(self.on.leader_elected, self.on_leader_elected)
@@ -747,8 +746,8 @@ class RecordingCharm(CharmBase):
 class RelationEventCharm(RecordingCharm):
     """Record events related to relation lifecycles."""
 
-    def __init__(self, framework, charm_name):
-        super().__init__(framework, charm_name)
+    def __init__(self, framework):
+        super().__init__(framework)
 
     def observe_relation_events(self, relation_name):
         self.framework.observe(self.on[relation_name].relation_created, self._on_relation_created)
@@ -913,7 +912,3 @@ class TestTestingModelBackend(unittest.TestCase):
         with self.assertRaises(RuntimeError):
             harness.add_oci_resource("missing-resource")
         self.assertEqual(len(harness._backend._resources_map), 0)
-
-
-if __name__ == "__main__":
-    unittest.main()