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

Use 'fail_json_aws' in the tags_need_modify check #211

Merged
merged 2 commits into from
Dec 1, 2020

Conversation

aplummerunsw
Copy link
Contributor

In reference to issue #210 ec2_group: error returned does not return AWS insufficient permissions error when adding tags

SUMMARY

This is the fix described in issue #210

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

amazon.aws/plugins/modules/ec2_group.py

ADDITIONAL INFORMATION

Change detailed in issue #210


In reference to issue ansible-collections#210 ec2_group: error returned does not return AWS insufficient permissions error when adding tags
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.

Thanks for submitting this PR. This looks like the right fix. Please add a changelog fragment:
https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to

Something like

bugfixes:
- ec2_group - Fixes error handling during tagging failures (https://github.com/ansible-collections/amazon.aws/issues/210).

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.

Thanks for that.

LGTM, I'll merge once the integration tests pass.

@aplummerunsw
Copy link
Contributor Author

I'll get the hang of these PRs eventually. Thanks. Maybe a silly question but how would this bugfix get into the 2.9 release?

@tremble
Copy link
Contributor

tremble commented Dec 1, 2020

Maybe a silly question but how would this bugfix get into the 2.9 release?

Not a silly question.

  1. Merged into here.
  2. Open a backport PR against 2.9, it's currently a little messy due to the way the split from all-in-one-repo to core vs collections. For example Partial backport of 60552 to fix ansible-collections/community.aws/198 ansible/ansible#71416

I'll handle the backport once the change is available here.

@aplummerunsw
Copy link
Contributor Author

Great, and thanks for the explanation! I really should get more involved with this and other OpenSource code contribution. I'll read the docs more thoroughly as well ;)

@tremble
Copy link
Contributor

tremble commented Dec 1, 2020

If you're interested in contributing more often, then it's worth having a read of https://www.ansible.com/blog/getting-started-with-aws-ansible-module-development

@tremble tremble merged commit 7b04f14 into ansible-collections:main Dec 1, 2020
@aplummerunsw
Copy link
Contributor Author

It says I can delete my fork of this repo but I have read that can show up as "unknown repository" in the PR's, what is the preferred practice?

@tremble
Copy link
Contributor

tremble commented Dec 1, 2020

Personally I go through and delete the extra branches but keep the fork (it's not like GitHub charges for a fork)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants