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

Fix column uppercasing #569

Merged
merged 34 commits into from
Oct 6, 2023
Merged

Conversation

kmarse
Copy link
Contributor

@kmarse kmarse commented Sep 6, 2023

SUMMARY

This pull request is to add logic to remove uppercasing column level privileges as they are case sensitive per MySQL databases.

This fixes #399

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

plugins/modules/mysql_role.py
plugins/modules/mysql_user.py
plugins/module_utils/user.py

ADDITIONAL INFORMATION

@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (8c2b6b0) 76.16% compared to head (ea60550) 74.40%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #569      +/-   ##
==========================================
- Coverage   76.16%   74.40%   -1.77%     
==========================================
  Files          28       18      -10     
  Lines        2379     2266     -113     
  Branches      579      564      -15     
==========================================
- Hits         1812     1686     -126     
- Misses        391      407      +16     
+ Partials      176      173       -3     
Flag Coverage Δ
integration 73.83% <100.00%> (+0.18%) ⬆️
sanity ?
units ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
plugins/module_utils/user.py 84.87% <100.00%> (+0.12%) ⬆️
plugins/modules/mysql_role.py 75.00% <100.00%> (-6.17%) ⬇️
plugins/modules/mysql_user.py 83.78% <100.00%> (-0.33%) ⬇️

... and 17 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Andersson007
Copy link
Collaborator

@kmarse hello, thanks for the PR!
Could you please:

  • fix sanity errors
  • add some integration tests for user and role to tests/integration/targets/*
  • add a changelog fragment

if you have any questions, please ask

@laurent-indermuehle
Copy link
Collaborator

@Andersson007 I feel like we should change the default in 4.0 to true ! We could announce it like you did in #408. Are you ok?

@kmarse, hope you're doing well with everything on your plate recently. I've got the integration tests completed for the changes in #399 - they're all passing. I want to get this merged as soon as we can as #572 is blocked by it. When you get a chance, would you mind taking a look at either granting me push access to your fork's main branch so I can add the test files? Otherwise I can open a PR there but it's less easy. I appreciate you taking the time.

If you think you won't have bandwidth for it in the next day or two though, feel free to let me know and I can open a PR on my end to move things along. Just wanted to check in first since it's your code. Thanks so much!

@kmarse
Copy link
Contributor Author

kmarse commented Oct 2, 2023

@laurent-indermuehle I have added you to my fork am just waiting your acceptance now. Sorry I am pretty busy on other projects and don't have the bandwidth to help a lot on this.

@laurent-indermuehle
Copy link
Collaborator

@kmarse thanks for the invitation, I accepted it. I'll try to push my tests tomorrow.

@Andersson007
Copy link
Collaborator

@Andersson007 I feel like we should change the default in 4.0 to true ! We could announce it like you did in #408. Are you ok?

@laurent-indermuehle yeah, i'm fine with that, thanks

laurent-indermuehle and others added 13 commits October 3, 2023 11:19
* Add MAX_STATEMENT_TIME to resource_limits

* Move version check for resource_limits to implementations
…ons#551)

* only use the "database" connection argument with driver versions where "db" is deprecated/removed

* connection arguments: fix KeyError

* connection arguments: fix KeyError

* connection arguments: use 'passwd' instead of 'password' with older drivers

* add changelog fragment

* refactoring: use "get_connector_name" in "mysql_connect"

---------

Co-authored-by: Felix Hamme <felix.hamme@ionos.com>
)

* fix connection arguments for MySQLdb <2.0 !=1.0

* add changelog fragment

---------

Co-authored-by: Felix Hamme <felix.hamme@ionos.com>
@laurent-indermuehle
Copy link
Collaborator

I don't get why this PR has been closed.
Maybe it's because kmarse was on 3.7.0 and our main on 3.7.2. I rebased and pushed again.

@laurent-indermuehle laurent-indermuehle changed the title Fixing column uppercasing Fix column uppercasing Oct 3, 2023
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.

@laurent-indermuehle thanks for continuing the work!
How about:

  • Adding yourself and @kmarse to the authors: ?
  • Adding a warning when the field is not set?

changelogs/569_fix_column_uppercasing.yml Outdated Show resolved Hide resolved
changelogs/569_fix_column_uppercasing.yml Outdated Show resolved Hide resolved
plugins/modules/mysql_role.py Outdated Show resolved Hide resolved
plugins/modules/mysql_role.py Outdated Show resolved Hide resolved
laurent-indermuehle and others added 7 commits October 5, 2023 10:28
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
This prevent forgetting the comma when adding more items to the list

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
@laurent-indermuehle
Copy link
Collaborator

@laurent-indermuehle thanks for continuing the work! How about:

  • Adding yourself and @kmarse to the authors: ?
  • Adding a warning when the field is not set?

@Andersson007 module.params['column_case_sensitive'] seems to be always sets.

Then I made a simple test "if not column_case_sensitive". Hope that's ok: baf947d

plugins/modules/mysql_role.py Outdated Show resolved Hide resolved
plugins/modules/mysql_role.py Outdated Show resolved Hide resolved
plugins/modules/mysql_user.py Outdated Show resolved Hide resolved
plugins/modules/mysql_user.py Outdated Show resolved Hide resolved
laurent-indermuehle and others added 4 commits October 6, 2023 14:51
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
@laurent-indermuehle laurent-indermuehle merged commit 033b4c7 into ansible-collections:main Oct 6, 2023
40 of 41 checks passed
@Andersson007
Copy link
Collaborator

@kmarse @laurent-indermuehle thanks a lot for the contribution!

This pull request was closed.
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.

Column Level Privileges are capitalized when applied to DB instance
5 participants