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 mysql_user on_new_username IndexError #642

Conversation

laurent-indermuehle
Copy link
Collaborator

SUMMARY
  • mysql_user - Fixed an IndexError in the update_password functionality introduced in PR feat[mysql_info]: add 'users_info' filter #580 and released in community.mysql 3.8.0. If you used this functionality, please avoid versions 3.8.0 to 3.9.0.
  • mysql_user - Added a warning to update_password's on_new_username option if multiple accounts with the same username but different passwords exist.
  • Enable the integration tests for the update_password functionality of mysql_user. Weirdly, they were never included in the main.yml fail.

Fixes #641

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
  • module_utils
  • mysql_user
  • mysql_info
ADDITIONAL INFORMATION

Previously we were returning only the first row found. We need to be
able to see if there is a difference in the existing passwords.
This make it possible to use the same method from mysql_user to help
update_password retrieve existing password for all account with the same
username independently of their hostname. And from mysql_info to get
the password of a specif user using WHERE user = '' AND host = ''
@laurent-indermuehle
Copy link
Collaborator Author

@betanummeric, would you be able to test this PR on your infrastructure? Maybe in a test environment or in check mode to prevent damage. There might be cases not covered by the test suite, although I doubt it given how well thought out your tests are!

@laurent-indermuehle laurent-indermuehle changed the title Lie fix mysql user on new username Fix mysql_user on_new_username IndexError Jun 6, 2024
@betanummeric
Copy link
Member

I will try to review this week

@laurent-indermuehle
Copy link
Collaborator Author

laurent-indermuehle commented Jun 10, 2024

Thanks @betanummeric!
Few tests are failing and I don't know why. I'll try to investigate this week.
Specifically this one... It's weird because I didn't touched the mysql_db module:

TASK [test_mysql_db : attempt connection with newly created user (expect failure)] ***
fatal: [testhost]: FAILED! => {"changed": false, "msg": "unable to find /root/.my.cnf. Exception message: (2026, 'SSL connection error: SSL_CTX_set_default_verify_paths failed')"}
...ignoring

TASK [test_mysql_db : assert] **************************************************
skipping: [testhost] => {"changed": false, "false_condition": "connector_name == 'pymysql'", "skip_reason": "Conditional result was False"}

TASK [test_mysql_db : assert] **************************************************
fatal: [testhost]: FAILED! => {
    "assertion": "result is succeeded",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed"
}

Copy link

codecov bot commented Jun 10, 2024

Codecov Report

Attention: Patch coverage is 84.61538% with 4 lines in your changes missing coverage. Please review.

Project coverage is 67.52%. Comparing base (0bc3e3d) to head (dfb3ef9).
Report is 3 commits behind head on main.

Current head dfb3ef9 differs from pull request most recent head b651053

Please upload reports for the commit b651053 to get more accurate results.

Files Patch % Lines
plugins/module_utils/user.py 84.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #642       +/-   ##
===========================================
+ Coverage   23.87%   67.52%   +43.65%     
===========================================
  Files          29       15       -14     
  Lines        2739     2473      -266     
  Branches      687      643       -44     
===========================================
+ Hits          654     1670     +1016     
+ Misses       2025      589     -1436     
- Partials       60      214      +154     
Flag Coverage Δ
integration 67.32% <84.61%> (?)
sanity ?
units ?

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

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

@laurent-indermuehle
Copy link
Collaborator Author

I've merged #631, then merged it in this branch and tests are still green. @betanummeric I just need your benediction before we can merge this :)

plugins/module_utils/user.py Outdated Show resolved Hide resolved
plugins/module_utils/user.py Outdated Show resolved Hide resolved
plugins/module_utils/user.py Outdated Show resolved Hide resolved
@laurent-indermuehle
Copy link
Collaborator Author

@betanummeric this is ready for a last review please.
This is my last week before vacations. If you manage to find time this week, It would be great while it's still fresh. Otherwise, rendez-vous mid July.
After this is merged I feel like we could release 3.10.

Copy link
Member

@betanummeric betanummeric left a comment

Choose a reason for hiding this comment

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

looks good now :)

@betanummeric betanummeric merged commit 33e8754 into ansible-collections:main Jun 27, 2024
43 checks passed
@Andersson007
Copy link
Collaborator

@laurent-indermuehle @betanummeric thanks for the contribution!

@laurent-indermuehle laurent-indermuehle deleted the lie_fix_mysql_user_on_new_username branch July 17, 2024 13:42
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.

Create new account with update_password and on_new_username returns IndexError
3 participants