Allow default network mode, as required for windows containers. #59597

ksagle77 · 2019-07-25T15:32:28Z

SUMMARY

Windows containers require the network mode. In boto3, this is configured by not passing a network_mode paramter, but this mode is not supported by this module.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

cloud

ADDITIONAL INFORMATION

…ontainers.

ansibot · 2019-07-25T15:42:22Z

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

lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py:247:48: trailing-whitespace Trailing whitespace

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

lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py:247:49: W291 trailing whitespace

click here for bot help

ansibot · 2019-07-25T15:42:23Z

cc @Java1Guy @willthames
click here for bot help

ansibot · 2019-07-25T15:42:24Z

@ksagle77, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

ansibot · 2020-01-31T17:40:12Z

cc @jillr @s-hertel @tremble
click here for bot help

xp0 · 2020-02-05T07:55:56Z

This would be really nice to have in the next release

tremble · 2020-02-06T09:31:04Z

@xp0 @ksagle77

https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task_definition_parameters.html seems to show when not set it'll default to 'bridge' which is what this module also does. Could you provide some information to show when / why this is necessary?

ksagle77 · 2020-02-06T10:30:04Z

@tremble

Hey Mark,

The issue is that docker for windows has it's own networking mode (NAT), which requires that no network mode is selected. Choosing bridge causes it to fail. From https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task_definition_parameters.html:

Docker for Windows uses a different network mode (known as NAT) than Docker for Linux. When you register a task definition with Windows containers, you must not specify a network mode. If you use the AWS Management Console to register a task definition with Windows containers, you must choose the default network mode.

Kevin

tremble

A couple of minor things here.

Could you add a changelog fragment https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs
Please add something to the description to explain what 'default' does

tremble · 2020-02-06T10:31:58Z

lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py

@@ -62,7 +62,7 @@
            - C(awsvpc) mode was added in Ansible 2.5
        required: false
        default: bridge
-        choices: [ 'bridge', 'host', 'none', 'awsvpc' ]
+        choices: [ 'default', 'bridge', 'host', 'none', 'awsvpc' ]


I'd like to see the effect of 'default' documented in the description above.

ksagle77 · 2020-02-06T14:27:04Z

@tremble

Hey Mark,

I've added the change fragment and a description. Let me know if it should be more detailed.

Kevin

tremble

Some tiny tweaks in the wording, but generally looking good to me.

lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py

changelogs/fragments/59597-ecs-allow_default_network_mode.yml

ansibot · 2020-02-06T14:38:25Z

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py:274:48: trailing-whitespace: Trailing whitespace
lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py:336:78: bad-whitespace: Exactly one space required after comma         network_mode=dict(required=False, default='bridge', choices=['default','bridge', 'host', 'none', 'awsvpc'], type='str'),                                                                               ^

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py:274:49: W291: trailing whitespace
lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py:336:79: E231: missing whitespace after ','

click here for bot help

Co-Authored-By: Mark Chappell <mchappel@redhat.com>

lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py

tremble · 2020-02-06T14:49:48Z

lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py

@@ -298,7 +302,7 @@ def main():
        revision=dict(required=False, type='int'),
        force_create=dict(required=False, default=False, type='bool'),
        containers=dict(required=False, type='list'),
-        network_mode=dict(required=False, default='bridge', choices=['bridge', 'host', 'none', 'awsvpc'], type='str'),
+        network_mode=dict(required=False, default='bridge', choices=['default','bridge', 'host', 'none', 'awsvpc'], type='str'),


Suggested change

network_mode=dict(required=False, default='bridge', choices=['default','bridge', 'host', 'none', 'awsvpc'], type='str'),

network_mode=dict(required=False, default='bridge', choices=['default', 'bridge', 'host', 'none', 'awsvpc'], type='str'),

Oh, didn't realize that. Adding it back, thanks.

Co-Authored-By: Mark Chappell <mchappel@redhat.com>

lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py

Co-Authored-By: Mark Chappell <mchappel@redhat.com>

lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py

tremble · 2020-02-06T15:38:23Z

One last change (removing the duplicate text, unfortunately GitHub doesn't let you make suggestions against multiple lines yet) and then I think we're good to go.

Co-Authored-By: Mark Chappell <mchappel@redhat.com>

ksagle77 · 2020-02-06T16:21:17Z

Removed. Thanks for the help.

jillr

LGTM, tests pass locally, thanks very much for this patch @ksagle77!

tremble · 2020-02-07T17:44:53Z

LGTM

tremble · 2020-02-08T21:08:20Z

bot_status

ansibot · 2020-02-08T21:15:48Z

Components

changelogs/fragments/59597-ecs-allow_default_network_mode.yml
support: community
maintainers:

lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py
support: community
maintainers: Java1Guy jillr s-hertel tremble

Metadata

waiting_on: maintainer
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): 1
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 0
shipit_actors (maintainers or core team members): tremble
shipit_actors_other: []
automerge: automerge shipit test failed

click here for bot help

…ble#59597) * Allow default network option for network_mode, required for windows containers. * Add change fragment and param description * Update changelogs/fragments/59597-ecs-allow_default_network_mode.yml Co-Authored-By: Mark Chappell <mchappel@redhat.com> * Update lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py Co-Authored-By: Mark Chappell <mchappel@redhat.com> * Correct descriptions. Remove whitespaces. * Remove empty line. * Add single space after comma * Update lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py Co-Authored-By: Mark Chappell <mchappel@redhat.com> * Update lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py Co-Authored-By: Mark Chappell <mchappel@redhat.com> * Update lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py Co-Authored-By: Mark Chappell <mchappel@redhat.com> Co-authored-by: kevinsagle <44478308+kevinsagle@users.noreply.github.com> Co-authored-by: Mark Chappell <mchappel@redhat.com>

Allow default network option for network_mode, required for windows c…

e454d29

…ontainers.

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 8, 2019

ansibot removed the needs_triage Needs a first human triage before being processed. label Feb 6, 2020

tremble suggested changes Feb 6, 2020

View reviewed changes

ansibot added 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 Feb 6, 2020

ksagle77 force-pushed the devel branch from 2349551 to e454d29 Compare February 6, 2020 14:06

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Feb 6, 2020

Add change fragment and param description

c603c1f

tremble suggested changes Feb 6, 2020

View reviewed changes

lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py Outdated Show resolved Hide resolved

changelogs/fragments/59597-ecs-allow_default_network_mode.yml Outdated Show resolved Hide resolved

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Feb 6, 2020

ksagle77 and others added 5 commits February 6, 2020 09:44

Update changelogs/fragments/59597-ecs-allow_default_network_mode.yml

89f389f

Co-Authored-By: Mark Chappell <mchappel@redhat.com>

Update lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py

3bc53a7

Co-Authored-By: Mark Chappell <mchappel@redhat.com>

Correct descriptions. Remove whitespaces.

417a899

Remove empty line.

7960e8d

Add single space after comma

10a8d5c

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Feb 6, 2020

tremble suggested changes Feb 6, 2020

View reviewed changes

Update lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py

332313d

Co-Authored-By: Mark Chappell <mchappel@redhat.com>

tremble reviewed Feb 6, 2020

View reviewed changes

lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py Outdated Show resolved Hide resolved

Update lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py

93af53c

Co-Authored-By: Mark Chappell <mchappel@redhat.com>

tremble reviewed Feb 6, 2020

View reviewed changes

lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py Outdated Show resolved Hide resolved

Update lib/ansible/modules/cloud/amazon/ecs_taskdefinition.py

87034a3

Co-Authored-By: Mark Chappell <mchappel@redhat.com>

tremble approved these changes Feb 6, 2020

View reviewed changes

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 6, 2020

jillr approved these changes Feb 7, 2020

View reviewed changes

jillr merged commit b9d22b9 into ansible:devel Feb 10, 2020

ansible locked and limited conversation to collaborators Mar 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow default network mode, as required for windows containers. #59597

Allow default network mode, as required for windows containers. #59597

ksagle77 commented Jul 25, 2019

ansibot commented Jul 25, 2019

ansibot commented Jul 25, 2019

ansibot commented Jul 25, 2019

ansibot commented Jan 31, 2020

xp0 commented Feb 5, 2020

tremble commented Feb 6, 2020

ksagle77 commented Feb 6, 2020 •

edited

tremble left a comment

tremble Feb 6, 2020

ksagle77 commented Feb 6, 2020

tremble left a comment

ansibot commented Feb 6, 2020

tremble Feb 6, 2020

ksagle77 Feb 6, 2020

tremble commented Feb 6, 2020

ksagle77 commented Feb 6, 2020

jillr left a comment

tremble commented Feb 7, 2020

tremble commented Feb 8, 2020

ansibot commented Feb 8, 2020

	network_mode=dict(required=False, default='bridge', choices=['default','bridge', 'host', 'none', 'awsvpc'], type='str'),
	network_mode=dict(required=False, default='bridge', choices=['default', 'bridge', 'host', 'none', 'awsvpc'], type='str'),

Allow default network mode, as required for windows containers. #59597

Allow default network mode, as required for windows containers. #59597

Conversation

ksagle77 commented Jul 25, 2019

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibot commented Jul 25, 2019

ansibot commented Jul 25, 2019

ansibot commented Jul 25, 2019

ansibot commented Jan 31, 2020

xp0 commented Feb 5, 2020

tremble commented Feb 6, 2020

ksagle77 commented Feb 6, 2020 • edited

tremble left a comment

Choose a reason for hiding this comment

tremble Feb 6, 2020

Choose a reason for hiding this comment

ksagle77 commented Feb 6, 2020

tremble left a comment

Choose a reason for hiding this comment

ansibot commented Feb 6, 2020

tremble Feb 6, 2020

Choose a reason for hiding this comment

ksagle77 Feb 6, 2020

Choose a reason for hiding this comment

tremble commented Feb 6, 2020

ksagle77 commented Feb 6, 2020

jillr left a comment

Choose a reason for hiding this comment

tremble commented Feb 7, 2020

tremble commented Feb 8, 2020

ansibot commented Feb 8, 2020

ksagle77 commented Feb 6, 2020 •

edited