clarify port.mode paramter requiremets, fail if unmet #47938

dariko · 2018-11-01T17:55:06Z

Docker python library supports enpointspec.publishmode only since version 3.0.0, and ignores it in previous releases publishing the port via vip.

This PR clarifies the documentation and has the module fail if the publish.mode parameter is used with a docker python library version <3.0.0

ansibot · 2018-11-01T18:05:06Z

Hi @dariko, thank you for submitting this pull-request!

click here for bot help

ansibot · 2018-11-01T18:05:07Z

cc @akshay196 @danihodovic @felixfontein @jwitko @kassiansun @tbouvet
click here for bot help

lib/ansible/modules/cloud/docker/docker_swarm_service.py

felixfontein · 2018-11-01T20:41:47Z

lib/ansible/modules/cloud/docker/docker_swarm_service.py

@@ -457,7 +457,7 @@
 import time
 from ansible.module_utils.docker_common import DockerBaseClass
 from ansible.module_utils.docker_common import AnsibleDockerClient
-from ansible.module_utils.docker_common import HAS_DOCKER_PY_3
+from module_utils.docker_common import docker_version


Ah, sorry, I think it also needs ansible. (as all the others). I wrote that down from my head, and it looks like I forgot something :)

Also, why don't you combine the above lines to:

from ansible.module_utils.docker_common import ( DockerBaseClass, AnsibleDockerClient, docker_version, )

(Combining them is a matter of style, so feel free to ignore that :) It just came to my mind when I saw the lines...)

Right... this time I tested it before pushing 😄
I simply didn't know that syntax, It's definitely better.

Looks like CI does not like that syntax, it's throwing this error

02:49 Run command: /usr/bin/python test/sanity/validate-modules/validate-modules --format json --arg-spec lib/ansible/modules/cloud/docker/docker_swarm_service.py --base-branch origin/devel 02:50 ERROR: Found 1 validate-modules issue(s) which need to be resolved: 02:50 ERROR: lib/ansible/modules/cloud/docker/docker_swarm_service.py:0:0: E321 Exception attempting to import module for argument_spec introspection, 'cannot import name 'docker_version'' (75%)

I'll go back to the more extensive form

The problem is unrelated to this change, it was already caused by the change before (using docker_version instead of HAS_DOCKER_PY_3). But that can be easily fixed by setting docker_version to None if docker-py cannot be imported (see my comment below).

felixfontein · 2018-11-01T20:43:29Z

changelogs/fragments/47938-docker_swarm_service-requirements.yaml

@@ -0,0 +1,2 @@
+minor_changes:


How about making this a bugfix (i.e. bugfixes:)?

Just a bugfix or a minor change and a bugfix?

I would make it simply a bugfix. And maybe also add that the parameter was simply ignored for that docker-py version before this fix. (I can't really think of any reason why the old behavior would have been useful...)

felixfontein · 2018-11-01T21:11:38Z

About the failing tests: the problem is that docker_version is not defined when docker-py is not available. This would have been fixed in #46527, but that didn't got merged so far (some changes are pending). You can copy the docker_common.py changes from that PR over into your PR (it's only one non-empty line), that should fix the problem.

ansibot · 2018-11-01T21:11:50Z

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:458:0: ImportError: cannot import name docker_version

The test ansible-test sanity --test import --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:458:0: ImportError: cannot import name docker_version

The test ansible-test sanity --test import --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:458:0: ImportError: cannot import name 'docker_version'

The test ansible-test sanity --test import --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:458:0: ImportError: cannot import name 'docker_version'

The test ansible-test sanity --test import --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:458:0: ImportError: cannot import name 'docker_version' from 'ansible.module_utils.docker_common' (/root/ansible/test/runner/.tox/import/lib/ansible/module_utils/docker_common.py)

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:0:0: E321 Exception attempting to import module for argument_spec introspection, 'cannot import name 'docker_version''

click here for bot help

changelogs/fragments/47938-docker_swarm_service-requirements.yaml

ansibot · 2018-11-01T21:39:12Z

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:460:0: ImportError: cannot import name docker_version

The test ansible-test sanity --test import --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:460:0: ImportError: cannot import name docker_version

The test ansible-test sanity --test import --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:460:0: ImportError: cannot import name 'docker_version'

The test ansible-test sanity --test import --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:460:0: ImportError: cannot import name 'docker_version'

The test ansible-test sanity --test import --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:460:0: ImportError: cannot import name 'docker_version' from 'ansible.module_utils.docker_common' (/root/ansible/test/runner/.tox/import/lib/ansible/module_utils/docker_common.py)

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/cloud/docker/docker_swarm_service.py:0:0: E321 Exception attempting to import module for argument_spec introspection, 'cannot import name 'docker_version''

click here for bot help

felixfontein · 2018-11-07T13:33:33Z

@dariko now that #46527 has been merged, can you remove the rollback commit and rebase against devel? I think it should then work.

felixfontein · 2018-11-09T07:24:32Z

@dariko if this should go into Ansible 2.7.2, we should have it merged and a backport ready until upcoming Monday.

ansibot · 2018-11-10T06:57:07Z

@dariko this PR contains the following merge commits:

6fb5a56

Please rebase your branch to remove these commits.

click here for bot help

ansibot · 2018-11-10T06:57:08Z

@dariko This PR was evaluated as a potentially problematic PR for the following reasons:

More than 50 changed files.
More than 50 commits.

Such PR can only be merged by human. Contact a Core team member to review this PR on IRC: #ansible-devel on irc.freenode.net

click here for bot help

Co-Authored-By: dariko <dariko@users.noreply.github.com>

dariko · 2018-11-10T08:12:20Z

@felixfontein I rebased to devel, now the the tests run without errors, I can open a backport PR as soon as this is merged.

felixfontein

shitip

felixfontein · 2018-11-10T09:39:40Z

@dariko the backport should probably include #46527 (i.e. cherry-pick both merge commits from that PR and this PR).

felixfontein · 2018-11-10T09:49:37Z

shipit

felixfontein · 2018-11-10T09:49:53Z

shitip

typo of the year... :D

dariko · 2018-11-10T10:57:20Z

@felixfontein Yes, otherwise the tests will fail; I'll cherry-pick this PR merge commit and c062f37

gundalow · 2018-11-12T11:36:34Z

@dariko Thank you for the PR
@felixfontein Thank you for the review

Merged into devel

felixfontein · 2018-11-12T13:00:14Z

@dariko do you have time to do the backport today? If not, I can also do it this evening.

* clarify port.mode paramter requiremets, fail if unmet * changelog fragment * shorten too long line * remove unnecessary indentation * test version on docker_version for better maintainability * normalize imports * changelog fragment: minor_changes -> bugfixes * rollback e96a7e57dfefd566fa47cf465a759637affd4795 * typo Co-Authored-By: dariko <dariko@users.noreply.github.com> (cherry picked from commit 89bcd3f)

dariko · 2018-11-12T15:01:36Z

@felixfontein thank you for everything :) I created the backport PR #48565; I never opened a backport PR before.. I hope it is all right.
@gundalow thank you for merging

felixfontein · 2018-11-12T15:07:52Z

@dariko you're welcome! Creating a backport is usually pretty simple, after a few times you usually even remember the branch name format (took me some more times I think ;) ).

gundalow · 2018-11-12T15:54:10Z

@dariko Backport process https://docs.ansible.com/ansible/devel/community/development_process.html#backport-pull-request-process

dariko · 2018-11-12T16:55:22Z

@gundalow thank you, that's the guide i followed!

* clarify port.mode paramter requiremets, fail if unmet * changelog fragment * shorten too long line * remove unnecessary indentation * test version on docker_version for better maintainability * normalize imports * changelog fragment: minor_changes -> bugfixes * rollback e96a7e57dfefd566fa47cf465a759637affd4795 * typo Co-Authored-By: dariko <dariko@users.noreply.github.com>

* clarify port.mode paramter requiremets, fail if unmet * changelog fragment * shorten too long line * remove unnecessary indentation * test version on docker_version for better maintainability * normalize imports * changelog fragment: minor_changes -> bugfixes * rollback e96a7e57dfefd566fa47cf465a759637affd4795 * typo Co-Authored-By: dariko <dariko@users.noreply.github.com> (cherry picked from commit 89bcd3f)

* clarify port.mode paramter requiremets, fail if unmet * changelog fragment * shorten too long line * remove unnecessary indentation * test version on docker_version for better maintainability * normalize imports * changelog fragment: minor_changes -> bugfixes * rollback e96a7e57dfefd566fa47cf465a759637affd4795 * typo Co-Authored-By: dariko <dariko@users.noreply.github.com>

samdoran removed the needs_triage Needs a first human triage before being processed. label Nov 1, 2018

felixfontein reviewed Nov 1, 2018

View reviewed changes

lib/ansible/modules/cloud/docker/docker_swarm_service.py Outdated Show resolved Hide resolved

felixfontein reviewed Nov 1, 2018

View reviewed changes

dariko force-pushed the docker_swarm_service_clarify_publish_requirements branch from 7590f7c to ce6f74c Compare November 1, 2018 20:43

ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Nov 1, 2018

felixfontein reviewed Nov 1, 2018

View reviewed changes

changelogs/fragments/47938-docker_swarm_service-requirements.yaml Outdated Show resolved Hide resolved

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Nov 1, 2018

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Nov 1, 2018

felixfontein mentioned this pull request Nov 3, 2018

Bug 42787 create volume with label #46527

Merged

ansibot added merge_commit This PR contains at least one merge commit. Please resolve! and removed ci_verified Changes made in this PR are causing tests to fail. labels Nov 10, 2018

typo

750a3e7

Co-Authored-By: dariko <dariko@users.noreply.github.com>

dariko force-pushed the docker_swarm_service_clarify_publish_requirements branch from 3293cd7 to 750a3e7 Compare November 10, 2018 07:01

felixfontein approved these changes Nov 10, 2018

View reviewed changes

ansibot added shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Nov 10, 2018

gundalow approved these changes Nov 12, 2018

View reviewed changes

gundalow merged commit 89bcd3f into ansible:devel Nov 12, 2018

dariko mentioned this pull request Nov 12, 2018

Backport/2.7/47938 #48565

Merged

ansible locked and limited conversation to collaborators Jul 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clarify port.mode paramter requiremets, fail if unmet #47938

clarify port.mode paramter requiremets, fail if unmet #47938

dariko commented Nov 1, 2018

ansibot commented Nov 1, 2018

ansibot commented Nov 1, 2018

felixfontein Nov 1, 2018

felixfontein Nov 1, 2018

dariko Nov 1, 2018

dariko Nov 1, 2018

felixfontein Nov 1, 2018

felixfontein Nov 1, 2018

dariko Nov 1, 2018

felixfontein Nov 1, 2018

felixfontein commented Nov 1, 2018

ansibot commented Nov 1, 2018

ansibot commented Nov 1, 2018

felixfontein commented Nov 7, 2018

felixfontein commented Nov 9, 2018

ansibot commented Nov 10, 2018

ansibot commented Nov 10, 2018

dariko commented Nov 10, 2018 •

edited

felixfontein left a comment

felixfontein commented Nov 10, 2018

felixfontein commented Nov 10, 2018

felixfontein commented Nov 10, 2018

dariko commented Nov 10, 2018

gundalow commented Nov 12, 2018

felixfontein commented Nov 12, 2018

dariko commented Nov 12, 2018

felixfontein commented Nov 12, 2018

gundalow commented Nov 12, 2018

dariko commented Nov 12, 2018

clarify port.mode paramter requiremets, fail if unmet #47938

clarify port.mode paramter requiremets, fail if unmet #47938

Conversation

dariko commented Nov 1, 2018

ansibot commented Nov 1, 2018

ansibot commented Nov 1, 2018

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

felixfontein commented Nov 1, 2018

ansibot commented Nov 1, 2018

ansibot commented Nov 1, 2018

felixfontein commented Nov 7, 2018

felixfontein commented Nov 9, 2018

ansibot commented Nov 10, 2018

ansibot commented Nov 10, 2018

dariko commented Nov 10, 2018 • edited

felixfontein left a comment

Choose a reason for hiding this comment

felixfontein commented Nov 10, 2018

felixfontein commented Nov 10, 2018

felixfontein commented Nov 10, 2018

dariko commented Nov 10, 2018

gundalow commented Nov 12, 2018

felixfontein commented Nov 12, 2018

dariko commented Nov 12, 2018

felixfontein commented Nov 12, 2018

gundalow commented Nov 12, 2018

dariko commented Nov 12, 2018

dariko commented Nov 10, 2018 •

edited