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 common helper for ec2 client 'ensure_tags' #309

Merged
merged 6 commits into from
Apr 23, 2021

Conversation

tremble
Copy link
Contributor

@tremble tremble commented Mar 23, 2021

Depends-On: ansible/ansible-zuul-jobs#876

SUMMARY

We're copy&pasting our 'ensure_tags' code all over the place. Move the ensure_tags code for EC2 resources into module_utils

Note: It might work for other boto3 clients, but it's intended only for ec2 boto3 clients (unfortunately boto3 isn't consistent)

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ec2_vol
ec2_vpc_subnet
ec2_tag
ec2_tag_info

ADDITIONAL INFORMATION

Todo:

  • unit tests
  • changelog

@ansibullbot
Copy link

@ansibullbot ansibullbot added WIP Work in progress feature This issue/PR relates to a feature request module module module_utils module_utils needs_triage plugins plugin (any type) labels Mar 23, 2021
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.

I also had this in my TODO backlog so thanks very much for beating me to it @tremble :)
Unit tests would be most welcome.

plugins/module_utils/ec2.py Outdated Show resolved Hide resolved
@ansibullbot
Copy link

@tremble tremble changed the title [WIP] Add common helper for ec2 client 'ensure_tags' Add common helper for ec2 client 'ensure_tags' Apr 21, 2021
@tremble
Copy link
Contributor Author

tremble commented Apr 21, 2021

After discussion with @jillr we don't think it's worth trying to build unit tests, the code is thoroughly tested by integration tests

@ansibullbot ansibullbot added community_review and removed WIP Work in progress labels Apr 21, 2021
@tremble tremble requested a review from jillr April 21, 2021 18:37
@tremble
Copy link
Contributor Author

tremble commented Apr 21, 2021

@jillr if possible I'd like to get this in before 2.0.0, not bothered about 1.5.0

@tremble tremble requested review from goneri and alinabuzachis April 21, 2021 18:59
Copy link
Collaborator

@alinabuzachis alinabuzachis left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @tremble.

@tremble
Copy link
Contributor Author

tremble commented Apr 23, 2021

recheck

@tremble
Copy link
Contributor Author

tremble commented Apr 23, 2021

For some reason it failed to find community.aws.ec2_instance...

@alinabuzachis
Copy link
Collaborator

alinabuzachis commented Apr 23, 2021

For some reason it failed to find community.aws.ec2_instance...

@tremble I was just looking at your errors on Zuul. I got the same here #334. I talked with Jill yesterday and they suggested to try with the FQDN for the ec2_instance and ec2_instance_info. I wait for the CI now.

@tremble
Copy link
Contributor Author

tremble commented Apr 23, 2021

It's a little strange because it said it was redirecting it to community.aws.ec2_instance...

@alinabuzachis
Copy link
Collaborator

It's a little strange because it said it was redirecting it to community.aws.ec2_instance...

Yes, you are right... so this shouldn't be the problem. However, I tried to explicitly use the FQDN (it didn't help). I noticed the same error in the other PRs as well. I will try asking Gonéri later.

@goneri
Copy link
Member

goneri commented Apr 23, 2021

recheck

@goneri
Copy link
Member

goneri commented Apr 23, 2021

It's a little strange because it said it was redirecting it to community.aws.ec2_instance...

Yes, you are right... so this shouldn't be the problem. However, I tried to explicitly use the FQDN (it didn't help). I noticed the same error in the other PRs as well. I will try asking Gonéri later.

Indeed, there is nothing from a CI perspective that define the dependency between the two collections. I've pushed a fix to address the problem.

@jillr jillr added the gate label Apr 23, 2021
@ansible-zuul ansible-zuul bot merged commit 042b8be into ansible-collections:main Apr 23, 2021
@tremble tremble deleted the ensure_ec2_tags branch May 28, 2021 07:07
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Oct 24, 2023
Update module name deprecation messages to be more helpful
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community_review feature This issue/PR relates to a feature request module_utils module_utils module module plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants