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

Deprecate REQUIRESSL privilege #132

Merged

Conversation

Jorge-Rodriguez
Copy link
Contributor

SUMMARY

Parse the priv string to effectively turn the REQUIRESSL privilege into an alias for the equivalent setup in the tls_requires option.
Introduce a deprecation notice for the REQUIRESSL privilege.

Fixes #89 #121

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

mysql_user

@codecov
Copy link

codecov bot commented Mar 30, 2021

Codecov Report

Merging #132 (35d8af5) into main (ecd15c8) will increase coverage by 0.51%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #132      +/-   ##
==========================================
+ Coverage   76.61%   77.13%   +0.51%     
==========================================
  Files          20       20              
  Lines        1770     1784      +14     
  Branches      436      436              
==========================================
+ Hits         1356     1376      +20     
+ Misses        268      265       -3     
+ Partials      146      143       -3     
Impacted Files Coverage Δ
plugins/modules/mysql_user.py 82.58% <100.00%> (+1.43%) ⬆️
tests/unit/plugins/modules/test_mysql_user.py 100.00% <100.00%> (ø)

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 ecd15c8...35d8af5. Read the comment docs.

@Andersson007
Copy link
Collaborator

ERROR: Found 4 pep8 issue(s) which need to be resolved:
ERROR: plugins/modules/mysql_user.py:975:40: E226: missing whitespace around arithmetic operator
ERROR: plugins/modules/mysql_user.py:978:40: E226: missing whitespace around arithmetic operator
ERROR: plugins/modules/mysql_user.py:978:61: E226: missing whitespace around arithmetic operator
ERROR: plugins/modules/mysql_user.py:978:128: E226: missing whitespace around arithmetic operator

maybe something else in CI

plugins/modules/mysql_user.py Outdated Show resolved Hide resolved
plugins/modules/mysql_user.py Show resolved Hide resolved
plugins/modules/mysql_user.py Outdated Show resolved Hide resolved
@Jorge-Rodriguez
Copy link
Contributor Author

Codecov is killing my OCD 😄

@Jorge-Rodriguez
Copy link
Contributor Author

Ok, I was happily assuming that I wrote flawless code, but the fact that the integration tests were green when I removed all references to REQUIRESSL and forgot to invoke the new function would suggest that we weren't testing (properly) for REQUIRESSL so let's not merge this until I have proper tests in place.

@Andersson007
Copy link
Collaborator

Ok, I was happily assuming that I wrote flawless code, but the fact that the integration tests were green when I removed all references to REQUIRESSL and forgot to invoke the new function would suggest that we weren't testing (properly) for REQUIRESSL so let's not merge this until I have proper tests in place.

Sure, let me know when it's ready for review

@Jorge-Rodriguez
Copy link
Contributor Author

Ping @kkeane @Andersson007

@Andersson007
Copy link
Collaborator

@Jorge-Rodriguez should we review it now or better to wait until the CI is green?

@Jorge-Rodriguez
Copy link
Contributor Author

@Andersson007 let's wait. I think I've spotted the issue with the failed CI and it's a systemic one on the module utils. I'm going to try to fix it and piggyback it on this PR

@Jorge-Rodriguez
Copy link
Contributor Author

@Andersson007 ok, one changed my mind. This is ready for review.

I checked the issue and the fix is not as straightforward as I initially estimated and it warrants A PR of it's own.

I'm going to open a discussion about it based on this comment

@Andersson007
Copy link
Collaborator

@bmalynovytch your thoughts on this?

Copy link
Contributor

@bmalynovytch bmalynovytch left a comment

Choose a reason for hiding this comment

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

LGTM

@Jorge-Rodriguez
Copy link
Contributor Author

I will change the tests to what @Andersson007 suggested and that should make integration green before merging.

@Andersson007
Copy link
Collaborator

@Jorge-Rodriguez so, we will wait for the tests changes mentioned, right?

@Jorge-Rodriguez
Copy link
Contributor Author

@Jorge-Rodriguez so, we will wait for the tests changes mentioned, right?

@Andersson007 yes, I'll ping you again when it's ready.

@Jorge-Rodriguez
Copy link
Contributor Author

Ping @Andersson007
Ready

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
@Jorge-Rodriguez thanks for the great work!

@Andersson007 Andersson007 merged commit dc522cc into ansible-collections:main Apr 10, 2021
@Andersson007
Copy link
Collaborator

So, we're ready to conduct our first major release soon. Maybe next week, say, on Monday:) will see

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_user idempotency issue with REQUIRESSL and ALL privileges
3 participants