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

ec2_asg: Change purge_tags default value to False #1064

Conversation

mandar242
Copy link
Contributor

SUMMARY

Changed default value of purge_tags to False.

With the addition of purge_tags to ec2_asg module #960, the default value was kept to True similar to many other modules in this collection and also as specified in ansible dev guide.

With this addition, the issue was discovered that there is a possibility of this change breaking existing playbooks for users if they don't update their playbooks and specify purge_tags: false where required.

This PR's change will prevent accidental breakage.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ec2_asg

@ansibullbot
Copy link

ansibullbot commented Apr 13, 2022

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review integration tests/integration module module needs_triage plugins plugin (any type) tests tests labels Apr 13, 2022
@softwarefactory-project-zuul

This comment was marked as outdated.

@mandar242
Copy link
Contributor Author

recheck

@softwarefactory-project-zuul

This comment was marked as outdated.

@mandar242
Copy link
Contributor Author

recheck

@markuman
Copy link
Member

markuman commented Apr 14, 2022

@mandar242 @alinabuzachis @jillr once this is merged, do we ship it with community.aws 3.2.1.
otherwise it is a breaking change - even if it is not a worse one - imo.

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.

Let's undo this breaking change, but also look at how we want to do this. About 3 quarters of our modules that support managing tags default to purge_tags=True behaviour today (some even nuke tags if you don't set the tags parameter)

@markuman
Copy link
Member

Let's undo this breaking change, but also look at how we want to do this. About 3 quarters of our modules that support managing tags default to purge_tags=True behaviour today (some even nuke tags if you don't set the tags parameter)

I put this to the agenda for the next meeting.
So we're going to release 3.2.1 once it is merged?

@markuman markuman added the backport-3 PR should be backported to the stable-3 branch label Apr 14, 2022
@markuman markuman added the mergeit Merge the PR (SoftwareFactory) label Apr 14, 2022
@markuman
Copy link
Member

regate

@felixfontein
Copy link
Contributor

Shouldn't this have a changelog fragment so that users know what happens?

@markuman
Copy link
Member

Shouldn't this have a changelog fragment so that users know what happens?

yeah. good point. It can be added in the release PR, which also needs to be approved and where some extra notes should be left in the release_summary

@markuman markuman removed the mergeit Merge the PR (SoftwareFactory) label Apr 14, 2022
@markuman
Copy link
Member

I've pushed a changelog fragment. It needs some reviews again @tremble @marknet15 @felixfontein

@tremble tremble added the mergeit Merge the PR (SoftwareFactory) label Apr 14, 2022
@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit d0c9f1a into ansible-collections:main Apr 14, 2022
@patchback
Copy link

patchback bot commented Apr 14, 2022

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/d0c9f1a6ee6198358a4ab2e59266e17ac10bfe25/pr-1064

Backported as #1069

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Apr 14, 2022
ec2_asg: Change purge_tags default value to False

SUMMARY

Changed default value of purge_tags to False.

With the addition of purge_tags to ec2_asg module #960, the default value was kept to True similar to many other modules in this collection and also as specified in ansible dev guide.
With this addition, the issue was discovered that there is a possibility of this change breaking existing playbooks for users if they don't update their playbooks and specify purge_tags: false where required.
This PR's change will prevent accidental breakage.
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

ec2_asg

Reviewed-by: Mark Chappell <None>
Reviewed-by: Markus Bergholz <[email protected]>
Reviewed-by: Mark Woolley <[email protected]>
Reviewed-by: Alina Buzachis <None>
(cherry picked from commit d0c9f1a)
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Apr 14, 2022
[PR #1064/d0c9f1a6 backport][stable-3] ec2_asg: Change purge_tags default value to False

This is a backport of PR #1064 as merged into main (d0c9f1a).
SUMMARY

Changed default value of purge_tags to False.

With the addition of purge_tags to ec2_asg module #960, the default value was kept to True similar to many other modules in this collection and also as specified in ansible dev guide.
With this addition, the issue was discovered that there is a possibility of this change breaking existing playbooks for users if they don't update their playbooks and specify purge_tags: false where required.
This PR's change will prevent accidental breakage.
ISSUE TYPE


Bugfix Pull Request

COMPONENT NAME

ec2_asg

Reviewed-by: Mark Chappell <None>
@mandar242 mandar242 deleted the ec2_asg_purge_tags_default_fix branch June 30, 2022 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-3 PR should be backported to the stable-3 branch bug This issue/PR relates to a bug community_review has_issue integration tests/integration mergeit Merge the PR (SoftwareFactory) module module plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants