Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tags to aws ecs task #53717

Open
wants to merge 8 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@ezmac
Copy link
Contributor

ezmac commented Mar 13, 2019

SUMMARY

Adds support for tags to ecs_task. Partially addresses #51122

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ecs_task

ADDITIONAL INFORMATION

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 13, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 13, 2019

@ezmac, 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

@@ -66,6 +66,11 @@
required: false
version_added: 2.8
choices: ["EC2", "FARGATE"]
task_tags:

This comment has been minimized.

@willthames

willthames Mar 13, 2019

Contributor

Can this just be tags for consistency with every other AWS module?

This comment has been minimized.

@ezmac

ezmac Mar 13, 2019

Author Contributor

It should have been. My syntax highlighter got confused and I did as well.

Thanks for your time reviewing and the suggestions you gave.

if (self.ecs_task_long_format_enabled()):
params['tags'] = ansible_dict_to_boto3_tag_list(tags, 'key', 'value')
else:
self.module.fail_json(msg="Account does not support long format task arns")

This comment has been minimized.

@willthames

willthames Mar 13, 2019

Contributor

I get that long format task ARNS might be a prerequisite for tags but might be better say e.g.

"Cannot set task tags: long format task arns are required to set tags"

@@ -339,6 +370,9 @@ def main():
if module.params['launch_type'] and not service_mgr.ecs_api_handles_launch_type():
module.fail_json(msg='botocore needs to be version 1.8.4 or higher to use launch type')

if module.params['task_tags'] and not service_mgr.ecs_api_handles_tags():
module.fail_json(msg='botocore needs to be version 1.12.46 or higher to use tags')

This comment has been minimized.

@willthames

willthames Mar 13, 2019

Contributor

Probably nicer to use

msg=missing_required_lib("botocore >= 1.12.46", reason="to use tags")
fix review items for ecs_task tags
use missing_required_lib for tags
change fail_json message to suggested message
switch from task_tags to tags for consisitency
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 13, 2019

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

lib/ansible/modules/cloud/amazon/ecs_task.py:372:33: undefined-variable Undefined variable 'missing_required_lib'

click here for bot help

@ezmac

This comment has been minimized.

Copy link
Contributor Author

ezmac commented Mar 13, 2019

I also moved the check for account support to the same block as the check for boto support so that there's only one place an error is defined. if you'd rather have it the other way, please let me know.

@willthames

This comment has been minimized.

Copy link
Contributor

willthames commented Mar 14, 2019

Looks good now @ezmac - I'll run this through the test suite in the next couple of days and it should be good to merge.

Combine testing policies
Because of the maximum of 10 policies per group, need to
consolidate testing policies as best we can.
state: present
name: awscli

- name: disable taskLongArnFormat

This comment has been minimized.

@willthames

willthames Mar 17, 2019

Contributor

This can be expressed far more healthily (i.e. you can see what has gone wrong because you don't have to use no_log) as:

      command: aws ecs put-account-setting --name taskLongArnFormat --value disabled
      environment:
        AWS_ACCESS_KEY_ID: "{{ aws_access_key }}"
        AWS_SECRET_ACCESS_KEY: "{{ aws_secret_key }}"
        AWS_SESSION_TOKEN: "{{ security_token }}"
        AWS_DEFAULT_REGION: "{{ aws_region }}"

This comment has been minimized.

@willthames

willthames Mar 17, 2019

Contributor

I'll probably just push up my fix for this shortly, so don't worry too much about it :)

Tidy put-account-setting tasks and add permission
Using `environment` and `command` rather than `shell` avoids the
need for `no_log` and means that people can fix the problem
@willthames

This comment has been minimized.

Copy link
Contributor

willthames commented Mar 17, 2019

I've added a couple of commits that make tests pass

There are some comprehensive changes to the testing policies due to the limit of 10 policies per group - so need to consolidate policies to get back under 10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.