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

mysql_role: add argument "members_must_exist" #369

Conversation

betanummeric
Copy link
Member

@betanummeric betanummeric commented May 23, 2022

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

mysql_role

SUMMARY

Add a new argument "members_must_exist" (boolean, default true).

The assertion that the users supplied in the "members" argument exist is only executed when the new argument "members_must_exist" is true, to allow opt-out.

fixes #366 (the "raising too much errors" part)

@betanummeric betanummeric marked this pull request as ready for review May 23, 2022 13:03
@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #369 (eea57da) into main (6474610) will decrease coverage by 0.02%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main     #369      +/-   ##
==========================================
- Coverage   78.12%   78.10%   -0.03%     
==========================================
  Files          27       27              
  Lines        2272     2279       +7     
  Branches      549      552       +3     
==========================================
+ Hits         1775     1780       +5     
- Misses        339      340       +1     
- Partials      158      159       +1     
Impacted Files Coverage Δ
plugins/modules/mysql_role.py 85.66% <75.00%> (-0.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6474610...eea57da. Read the comment docs.

Copy link
Collaborator

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

@betanummeric thanks very much for fixing and improving the module! I like the idea.
Speaking about the default value, we can't merge it now if the it's yes as it would be a breaking change (we are following SemVer and such changes must be announced in advance with warnings, etc., and released in a major release).

I suggest making it no for now. We could discuss and change it later in a separate issue. If the community will be OK, we'll create a PR which we'll merge before the next major release (which i don't think will happen earlier than in half a year).

So to proceed now, I suggest making it no by default (for now).

plugins/modules/mysql_role.py Outdated Show resolved Hide resolved
plugins/modules/mysql_role.py Show resolved Hide resolved
@betanummeric
Copy link
Member Author

@Andersson007 The current default members_must_exist: yes should preserve the behavior, so this should not be a breaking change. I previously had the variable named ignore_missing_members with the value flipped (default no), but I thought it was an unnecessary negation and renamed it to members_must_exist.

@Andersson007
Copy link
Collaborator

@Andersson007 The current default members_must_exist: yes should preserve the behavior, so this should not be a breaking change. I previously had the variable named ignore_missing_members with the value flipped (default no), but I thought it was an unnecessary negation and renamed it to members_must_exist.

Ah, got it now, busy days... sorry:)
OK, let's wait for folks' opinions for a day or two (fail_on_member came to mind because there's a (not fully similar) parameter called fail_on_user in the postgresql_user module).
Could you also please resolve the conflict (https://docs.ansible.com/ansible/latest/dev_guide/developing_rebasing.html use main instead of devel)

@betanummeric betanummeric force-pushed the mysql_role-add-members_must_exist-argument branch from 82e1553 to eea57da Compare May 25, 2022 15:27
@Andersson007
Copy link
Collaborator

@betanummeric there's an integration test error in case you've missed it

@betanummeric
Copy link
Member Author

@Andersson007 Thanks for the hint, but it looks like the error is unrelated to my changes:

Run pip install https://github.com/ansible/ansible/archive/stable-2.13.tar.gz --disable-pip-version-check
Collecting https://github.com/ansible/ansible/archive/stable-2.13.tar.gz
  WARNING: Retrying (Retry(total=4, connect=None, read=None, redirect=None, status=None)) after connection broken by 'ReadTimeoutError("HTTPSConnectionPool(host='github.com', port=443): Read timed out. (read timeout=15)")': /ansible/ansible/archive/stable-2.13.tar.gz
  ERROR: HTTP error 502 while getting https://github.com/ansible/ansible/archive/stable-2.13.tar.gz
ERROR: Could not install requirement https://github.com/ansible/ansible/archive/stable-2.13.tar.gz because of HTTP error 502 Server Error: Bad Gateway for url: https://github.com/ansible/ansible/archive/stable-2.13.tar.gz for URL https://github.com/ansible/ansible/archive/stable-2.13.tar.gz

All other integration tests ran successfully. Can you maybe reschedule the failed test?

@Andersson007
Copy link
Collaborator

@betanummeric ah, OK, thanks. Unfortunately there's no way to re-launch only one task in GHA, so i closed reopened the PR to trigger another run

@Andersson007
Copy link
Collaborator

ah, GitHub has issues now https://www.githubstatus.com/. I'll relauch the tests later

Copy link
Collaborator

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

LGTM

@Andersson007 Andersson007 merged commit bf5086d into ansible-collections:main May 27, 2022
@Andersson007
Copy link
Collaborator

@betanummeric thanks for the improvement!
@rsicart thanks for reviewing!

@Andersson007
Copy link
Collaborator

@betanummeric ah, should we maybe add an example of the new param use to the EXAMPLES block? Would you like to submit a PR? (no changelog fragment needed)

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.

mysql_role: multiple bugs with detach_members
4 participants