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.

Sign up for GitHub

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

Jump to bottom

Implement tags support for AWS Route 53 objects #565

Merged
merged 7 commits into from
Oct 14, 2021
Merged

Implement tags support for AWS Route 53 objects #565

merged 7 commits into from
Oct 14, 2021

Conversation

badnetmask
Copy link
Contributor

SUMMARY

Implement tags support for AWS Route 53 objects.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

community.aws.route53
community.aws.route53_health_check

ADDITIONAL INFORMATION

Current AWS Route 53 modules does not support tagging objects. This is important for organization, ownership tracking and financial accounting on enterprise environments.

- name: Create zone example.com
  community.aws.route53_zone:
    zone: example.com
    tags:
      Owner: DevOps Team
    purge_tags: True

- name: Create www health check
  community.aws.route53_health_check:
    fqdn: www.example.com
    type: TCP
    port: 443
    tags:
      Owner: DevOps Team
    purge_tags: True

@badnetmask
Copy link
Contributor Author

There appears to be one pending error on ansible-test-cloud-integration-aws-py36, which I cannot reproduce, but given the message it seems to be related to the test account permissions.

raise error_class(parsed_response, operation_name)\nbotocore.exceptions.ClientError: An error occurred (AccessDenied) when calling the ListTagsForResource operation: User: arn:aws:sts::966509639900:assumed-role/ansible-core-ci-test-dev/dev=remote=goneri is not authorized to perform: route53:ListTagsForResource on resource: arn:aws:route53:::hostedzone/Z03802332J78O2JWLQPLE\n",

@badnetmask badnetmask mentioned this pull request May 5, 2021
Merged
@jillr
Copy link
Collaborator

jillr commented May 6, 2021

recheck

@badnetmask badnetmask mentioned this pull request May 8, 2021
Closed
jillr
jillr previously requested changes May 10, 2021
View reviewed changes
Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks pretty good, thanks @badnetmask. It would be nice to have some additional tests that change the tags for a resource and that test purge_tags with both true and false.

plugins/modules/route53_health_check.py Outdated Show resolved Hide resolved
plugins/modules/route53_health_check.py Outdated Show resolved Hide resolved
plugins/modules/route53_zone.py Show resolved Hide resolved
plugins/modules/route53_zone.py Show resolved Hide resolved
plugins/modules/route53_zone.py Show resolved Hide resolved
@badnetmask badnetmask requested a review from jillr May 10, 2021 21:10
markuman
markuman requested changes May 19, 2021
View reviewed changes
Copy link
Member

@markuman markuman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'd like only to see an addition purge_tag integration test, where existing tags will be replaced by other tags.

tests/integration/targets/route53_zone/tasks/main.yml Show resolved Hide resolved
markuman
markuman approved these changes May 19, 2021
View reviewed changes
Copy link
Member

@markuman markuman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@goneri
Copy link
Member

goneri commented May 27, 2021

recheck

@ansibullbot
Copy link

cc @jimbydamonk @minichate @s-hertel @tremble @wimnat @zimbatm
click here for bot help

@ansibullbot ansibullbot added community_review feature This issue/PR relates to a feature request has_issue integration tests/integration module module new_contributor Help guide this first time contributor new_plugin New plugin plugins plugin (any type) tests tests labels May 27, 2021
tremble
tremble requested changes Sep 26, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @badnetmask sorry for the delays here.

With route53_health_check being boto2 based I'm nervous to add changes, especially without integration tests.

I'd also like to see a couple of non-camel-case tagging examples (at least a "snake_case" example). There's some easy mistakes that can be made but hidden. If you remove the route53_health_check change and add a couple of additional tagging tests we can get this merged.

For route53_health_check I've got initial integration tests in #732 to support a migration to boto3 with #734 after which your route53 helper should be really easy to add tagging support into route53_health_check.

@ansibullbot
Copy link

cc @markuman
click here for bot help

@ansibullbot ansibullbot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Oct 12, 2021
@tremble tremble removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Oct 12, 2021
@alinabuzachis
Copy link
Contributor

recheck

@tremble tremble dismissed jillr’s stale review October 13, 2021 18:53

Issues addressed

@tremble tremble added the gate label Oct 14, 2021
ansible-zuul[bot]
ansible-zuul bot approved these changes Oct 14, 2021
View reviewed changes
Copy link
Contributor

@ansible-zuul ansible-zuul bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ansible-zuul ansible-zuul bot merged commit dadc872 into ansible-collections:main Oct 14, 2021
@badnetmask badnetmask deleted the route53_tags branch February 7, 2022 20:36
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request May 25, 2022
@jillr
Prep 2.1.0 release (ansible-collections#565)
013162a 
Prep 2.1.0 release

SUMMARY
Run add_docs, generate changelog
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
amazon.aws 2.1

Reviewed-by: Mark Chappell <None>
Reviewed-by: None <None>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alinabuzachis alinabuzachis alinabuzachis approved these changes

@ansible-zuul ansible-zuul[bot] ansible-zuul[bot] approved these changes

@tremble tremble tremble approved these changes

@jillr jillr Awaiting requested review from jillr

@markuman markuman Awaiting requested review from markuman

Assignees
No one assigned
Labels
community_review feature This issue/PR relates to a feature request has_issue integration tests/integration module module new_contributor Help guide this first time contributor new_plugin New plugin plugins plugin (any type) tests tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@badnetmask @jillr @goneri @ansibullbot @alinabuzachis @tremble @markuman