Refactor plugins #76

jmaupetit · 2018-07-06T14:45:06Z

Purpose

Current "plugins" implementation is far from academic. We should follow Ansible's recommendations to improve them.

Proposal

Switch the app lookup call to a real lookup plugin
Fix merge filter implementation (remove deepmerge and make our own)
Add tests for plugins
Automate tests execution in the CI
Add python code linters (pylint, flake8, isort)

sampaccoud · 2018-07-11T07:00:53Z

.circleci/config.yml

@@ -47,16 +55,17 @@ jobs:
            bin/build
            mkdir -p /tmp/docker/images
            docker save -o /tmp/docker/images/arnold.tar "${ARNOLD_IMAGE}"
-      # Store the built as an artifact that can be reused in other jobs
+      # Save built to the workspace so that it can be reused in other jobs


s/built/build?

I meant save the built to the workspace.

sampaccoud · 2018-07-11T07:01:36Z

.circleci/config.yml

+      - *docker_load
+      - run: bin/lint plugins
+
+  test-built:


s/built/build?

test the build 😉

sampaccoud · 2018-07-11T07:03:41Z

.circleci/config.yml

@@ -105,22 +129,54 @@ jobs:
              "${ARNOLD_IMAGE}" \
              ansible --version

+  # FIXME: we have deactivated plugins test coverage as the container user is
+  # not allowed to write to /app/.coverage and CircleCI does not allowed to
+  # mount volumes with "docker run" commands.


you're saying this and then below you're mounting /app/.pytest_cache?

It's not a classical volume it's a tmpfs. Concerning volumes, I think I cannot use them since we are in "dind" mode. We need to switch to the machine executor at least for this task.

So why can't you use a tmpfs for /app/.coverage also?

Because it's a file, not a directory.

sampaccoud · 2018-07-11T07:03:48Z

.circleci/config.yml

@@ -105,22 +129,54 @@ jobs:
              "${ARNOLD_IMAGE}" \
              ansible --version

+  # FIXME: we have deactivated plugins test coverage as the container user is
+  # not allowed to write to /app/.coverage and CircleCI does not allowed to


s/allowed/allow

🤔 it's the container user that is not allowed to

I nearly apologized... then I realized that there are 2 "allowed" in this sentence ^^

Oops. you're right.

sampaccoud · 2018-07-11T07:04:30Z

.circleci/config.yml

+      - lint-plugins:
+          requires:
+            - build
+      - test-built:


ok... maybe you really meant built instead of build...

sampaccoud · 2018-07-11T07:22:44Z

tests/units/plugins/filter/test_merge.py

+        assert deep_sort_dict(d) == expected
+
+    def test_deep_embedded_dict(self):
+        """Avoid testing recursivity"""


wrong docstring

Yes. Sorry for that.

sampaccoud · 2018-07-11T07:24:15Z

tests/units/plugins/filter/test_merge.py

+            "bar": ["a", "b", "c", "d"],
+        }
+
+        assert deep_sort_dict(d) == expected


you should add:

assert d == {
"foo": {"baz": [4, 1, 6, {"lol": ["x", "z", "y"]}]},
"bar": ["c", "b", "d", "a"],
}

Same on all tests?

Not if we test immutability separately?

sampaccoud · 2018-07-11T07:26:29Z

tests/units/plugins/filter/test_merge.py

+    maxDiff = None
+
+    def assertDictDeepEqual(self, a, b):
+        """Use a sorted json string to deep compare apps (dictionaries)"""


The docstring seems wrong. Here you are testing that they are pointing to the same reference, not that their json representation is the same.

Wow. I should have carefully check my docstring. This is an old dumb implementation.

sampaccoud · 2018-07-11T07:28:32Z

tests/units/plugins/filter/test_merge.py

+            )
+
+    def test_submitted_apps_name_is_not_empty(self):
+        """Submitted apps should have the same ``name`` key"""


copy/paste fail

sampaccoud · 2018-07-11T07:29:27Z

tests/units/plugins/filter/test_merge.py

+
+    def test_services_merge_with_empty_new_services(self):
+        """
+            Test app services merge with no services or volumes for tje new


sampaccoud · 2018-07-11T07:39:14Z

tests/units/plugins/filter/test_merge.py

+
+    def test_services_merge_without_duplicates(self):
+        """
+            Test app services merge does not creates duplicates if same items


s/creates/create

sampaccoud · 2018-07-11T08:00:45Z

tests/units/plugins/filter/test_merge.py

+                    "name": "baz",
+                    "configs": ["baz.conf"],
+                    "templates": ["baz/dc.yml", "baz/svc.yml", "baz/ep.yml"],
+                },


So the fun service is gone is it what you expect?

Yes, we take data from right (new) to left (base) if the app is defined in left (base), i.e. you cannot define a new app that is not listed as available (with files in apps/).

For the moment I just read the tests. Maybe it deserves a line in the docstring.
Then the concept could be explained in some documentation (in another PR is fine)?

Instead of using the pipe lookup with an executed python program output, we implemented our own lookup plugin. It should have already been this way from the start.

Following Ansible's guidelines, this work adds a fully fonctionnal test environment using pytest.

sampaccoud · 2018-07-11T11:28:04Z

tests/units/plugins/filter/test_merge.py

+        d = {"foo": 1, "bar": ["c", "b", "d", "a"]}
+        s = deep_sort_dict(d)
+
+        assert d != s


assertion is good but you didn't test that it does not modify d :-)
assert d == {"foo": 1, "bar": ["c", "b", "d", "a"]} ?

sampaccoud · 2018-07-11T11:29:19Z

plugins/lookup/apps.py

+            │   │   ├── dc.yml.j2
+            │   │   └── svc.yml.j2
+            │   ├── nginx
+            │   │   ├── _configs


I think singular is better: config

I see plural everywhere (volumes, templates, etc). Let's stick to it?

but config is "the config"? Pick what you prefer.

There may be more that once configuration for a service (e.g. edxapp cms + lms ngnix configurations).

sampaccoud · 2018-07-11T11:30:12Z

plugins/filter/merge.py

-    """Merge data from the "new" application to the "base" application"""
+    """
+        Merge data from the "new" application to the "base" application.
+        Services listed in "new" that does not exist in "base" will be ignored.


sampaccoud · 2018-07-11T11:32:10Z

plugins/filter/merge.py

+        for k in ("configs", "templates"):
+            if new_service.get(k) is None:
+                continue
+            base_service[k] = sorted(list(set(new_service[k] + base_service[k])))


maybe add a comment to explain that you do list(set( to deduplicate?

Switch to our own apps merge implementation to avoid duplicates in lists.

The merge_with_app filter reliability is a core part of the app discovery feature. It requires intense testing to ensure consistency of our whole strategy.

A cosmetic update to make job names more homogeneous in FUN's projects.

We now run unit tests of Arnold's plugins in the CI.

We now use pylint, isort and flake8 to lint our custom plugins. Unfortunately black cannot be used as we are running python 2.7 and it requires at least python 3.6.

* apply black-style formatting * fix flake8 and isort warnings

jmaupetit added feature WIP labels Jul 6, 2018

jmaupetit self-assigned this Jul 6, 2018

jmaupetit force-pushed the refactor-plugins branch 5 times, most recently from 1044434 to 6079b08 Compare July 10, 2018 15:09

jmaupetit changed the title ~~WIP: refactor plugins~~ Refactor plugins Jul 10, 2018

jmaupetit requested review from cchaudier and sampaccoud July 10, 2018 15:10

jmaupetit force-pushed the refactor-plugins branch from 6079b08 to 505040f Compare July 10, 2018 20:20

jmaupetit added needs review and removed WIP labels Jul 11, 2018

sampaccoud reviewed Jul 11, 2018

View reviewed changes

This was referenced Jul 11, 2018

Consider switching base image to ubuntu #78

Closed

Consider adding black linter #79

Open

jmaupetit added 2 commits July 11, 2018 11:47

🎨(apps) make apps lookup a lookup plugin

ab0beff

Instead of using the pipe lookup with an executed python program output, we implemented our own lookup plugin. It should have already been this way from the start.

✨(tests) add pytest environment

1b596e3

Following Ansible's guidelines, this work adds a fully fonctionnal test environment using pytest.

jmaupetit force-pushed the refactor-plugins branch from 85acc55 to 3913584 Compare July 11, 2018 09:58

sampaccoud reviewed Jul 11, 2018

View reviewed changes

sampaccoud approved these changes Jul 11, 2018

View reviewed changes

jmaupetit added 6 commits July 11, 2018 14:52

🐛(plugin) fix merge_with_app filter

0366847

Switch to our own apps merge implementation to avoid duplicates in lists.

✅(plugin) add tests for the merge plugin

ab99c7f

The merge_with_app filter reliability is a core part of the app discovery feature. It requires intense testing to ensure consistency of our whole strategy.

🎨(circle) use dashes in job names

bb6472c

A cosmetic update to make job names more homogeneous in FUN's projects.

👷(circle) add plugins tests in the CI

1bf342d

We now run unit tests of Arnold's plugins in the CI.

👷(circle) add python code linters

cc6b012

We now use pylint, isort and flake8 to lint our custom plugins. Unfortunately black cannot be used as we are running python 2.7 and it requires at least python 3.6.

🚨(plugin) fix new anstomlog plugin code style

3f8248b

* apply black-style formatting * fix flake8 and isort warnings

jmaupetit force-pushed the refactor-plugins branch from 11dda21 to 3f8248b Compare July 11, 2018 12:52

jmaupetit merged commit 4d2f0b4 into master Jul 11, 2018

jmaupetit deleted the refactor-plugins branch July 11, 2018 13:06

lunika mentioned this pull request Nov 30, 2018

⬆️ (python) use python 3 to run arnold's playbooks #171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor plugins #76

Refactor plugins #76

jmaupetit commented Jul 6, 2018 •

edited

sampaccoud Jul 11, 2018

jmaupetit Jul 11, 2018

sampaccoud Jul 11, 2018

jmaupetit Jul 11, 2018

sampaccoud Jul 11, 2018

jmaupetit Jul 11, 2018

sampaccoud Jul 11, 2018

jmaupetit Jul 11, 2018

sampaccoud Jul 11, 2018

jmaupetit Jul 11, 2018

sampaccoud Jul 11, 2018

jmaupetit Jul 11, 2018

sampaccoud Jul 11, 2018

sampaccoud Jul 11, 2018

jmaupetit Jul 11, 2018

sampaccoud Jul 11, 2018

jmaupetit Jul 11, 2018

sampaccoud Jul 11, 2018

jmaupetit Jul 11, 2018

sampaccoud Jul 11, 2018

sampaccoud Jul 11, 2018

sampaccoud Jul 11, 2018

sampaccoud Jul 11, 2018

jmaupetit Jul 11, 2018

sampaccoud Jul 11, 2018

sampaccoud Jul 11, 2018

sampaccoud Jul 11, 2018

jmaupetit Jul 11, 2018

sampaccoud Jul 11, 2018

jmaupetit Jul 11, 2018

sampaccoud Jul 11, 2018

sampaccoud Jul 11, 2018

Refactor plugins #76

Refactor plugins #76

Conversation

jmaupetit commented Jul 6, 2018 • edited

Purpose

Proposal

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmaupetit commented Jul 6, 2018 •

edited