Get rid of privs comparison

[rsicart]

SUMMARY

Fixes #234

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

user, mysql_user, mysql_role

ADDITIONAL INFORMATION

See #234 for more details.

[codecov]

Codecov Report

Merging #243 (5ed6680) into main (5522e45) will increase coverage by 0.38%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #243      +/-   ##
==========================================
+ Coverage   77.61%   77.99%   +0.38%     
==========================================
  Files          24       24              
  Lines        2171     2159      -12     
  Branches      510      508       -2     
==========================================
- Hits         1685     1684       -1     
+ Misses        319      309      -10     
+ Partials      167      166       -1     

Impacted Files	Coverage Δ
plugins/module_utils/user.py	83.02% <100.00%> (+1.31%)	⬆️
plugins/modules/mysql_role.py	80.39% <100.00%> (-0.07%)	⬇️
plugins/modules/mysql_user.py	82.97% <100.00%> (+3.38%)	⬆️

---

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 5522e45...5ed6680. Read the comment docs.

[rsicart]

For the moment I removed all priv comparison code.

There's already some try/catch blocks which catch mysql_client.Error errors at mysql_user module level. I was thinking to raise a custom exception inside privileges_grant() but I'm not sure that we need more detail than this, catching the error at module level seems already enough. What do you think about that?

[rsicart]

Also I'm new with ansible-test. I think that currently there's only mysql tests. How do you test mariadb with ansible-test? Do you have some tips for that?

I tried locally to set mariadb_install: true in setup_mysql target defaults, but I have some errors. It seems more complicated than that.

[Andersson007]

@rsicart thanks for doing this!
I'm on vacation this week. @Jorge-Rodriguez , @bmalynovytch could you please take a look?

[Andersson007]

@rsicart thanks for working on this!

If we see CI green, it would be great

[rsicart]

CI doesn't pass because of python2. I used a raise ... from ... which is python3 only syntax. I didn't fix it because perhaps it's time to go on and forget python 2?
This week I'm on holidays, I do not have my computer to fix this pr.

[Andersson007]

CI doesn't pass because of python2. I used a raise ... from ... which is python3 only syntax. I didn't fix it because perhaps it's time to go on and forget python 2? This week I'm on holidays, I do not have my computer to fix this pr.

Unfortunately we can't drop Python 2 support (at least in the near future) because this collection is included in Ansible package and it requires support of Python versions that supported by all supported Ansible versions:)
You could use just module.fail_json() instead, I think

[Andersson007]

We also don't test against MariaDB. It would be really great to add it to our CI (i'd say it's vital to have it). I created an issue recently where help_wanted #238. So at least for now manual testing against MariaDB will be required.

[Andersson007]

@rsicart thanks for working on this!
When it's ready for review, please let us know and:

• Eliminate Draft
• Add a changelog fragment
• Give feedback on my comment about CI:)

Thank you

[rsicart]

I agree that having tests for mariadb is vital.

I just tested manually using docker on my computer and different version of mysql (5.6, 5.7, 8.0) and mariadb (10.4, 10.5).

I tried to add INVALID privilege to user toto@% using mysql client, in order to view error messages returned by the database.

Then I checked what kind of exception is raised by mysql modules (pymysql, mysqldb) in order to catch them, and I wrote integration tests to complete.

But doing that every time is annoying, automated testing for mariadb should be available in CI too!

[Jorge-Rodriguez]

But doing that every time is annoying, automated testing for mariadb should be available in CI too!

There has been a plan to implement testing against MariaDB for quite some time now, but it is not quite a trivial task as it turns out so we haven't gotten around to it. Recent changes to the requirements for the testing versions have further delayed that task. I do have a mind to have a go at it if there's no progress on that front but I've been quite caught up otherwise. All in all, this is in the backlog.

[Andersson007]

@rsicart thanks for working on this! Sorry for the later reply, was on PTO. Everything looks good. Thank you for adding the tests!
@Jorge-Rodriguez thanks for reviewing and merging!

[Andersson007]

@rsicart @Jorge-Rodriguez @bmalynovytch i didn't notice that this PR marked with breaking_change. Actually this shouldn't affect users, right? What do you think if we release this in 3.0.0 (this or next week)?
I wouldn't wait for 4.0.0 as it hasn't been planned at all:)

[Jorge-Rodriguez]

I'm good with 3.0.0