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

Handle divergences between MySQL and MariaDB #103

Merged

Conversation

Jorge-Rodriguez
Copy link
Contributor

SUMMARY

This is an initial proposal on how to handle the growing divergences between MariaDB and MySQL while keeping the module codebase as clean as possible.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

mysql collection

ADDITIONAL INFORMATION

@Jorge-Rodriguez
Copy link
Contributor Author

@Andersson007 @bmalynovytch @bmildren This is my initial (non-working) proposal on the matter above. I'm submitting this draft for your review and comments before tackling further changes and making it actually functional. Thanks in advance for your feedback.

@Andersson007
Copy link
Collaborator

@Jorge-Rodriguez looks interesting.
it's not related to this PR: If you write to me a test email to aaklychkov at mail dot ru, it would be really great. I won't bother you requesting reviews, etc. via it, it's just for discussing general things rarely. Thanks!

@bmalynovytch
Copy link
Contributor

The approach looks good to me for now 👍

@Jorge-Rodriguez Jorge-Rodriguez force-pushed the mysql_mariadb_dychotomy branch from eee823f to e0ca425 Compare March 7, 2021 15:56
@Jorge-Rodriguez Jorge-Rodriguez marked this pull request as ready for review March 7, 2021 15:57
@Jorge-Rodriguez
Copy link
Contributor Author

I think it makes more sense to leave this PR as is, just laying out the boilerplate work for future development.
The current conditional branches for MySQL/MariaDB are already transitioned, but I would refrain from doing any further development now.

@codecov
Copy link

codecov bot commented Mar 7, 2021

Codecov Report

Merging #103 (23fedb7) into main (baea97d) will increase coverage by 0.38%.
The diff coverage is 90.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
+ Coverage   75.75%   76.14%   +0.38%     
==========================================
  Files          12       19       +7     
  Lines        1712     1748      +36     
  Branches      438      434       -4     
==========================================
+ Hits         1297     1331      +34     
- Misses        269      270       +1     
- Partials      146      147       +1     
Impacted Files Coverage Δ
tests/unit/plugins/modules/test_mysql_user.py 100.00% <ø> (ø)
plugins/modules/mysql_user.py 81.15% <54.54%> (+0.14%) ⬆️
...ugins/module_utils/implementations/mariadb/user.py 77.77% <77.77%> (ø)
...odule_utils/implementations/mariadb/replication.py 100.00% <100.00%> (ø)
.../module_utils/implementations/mysql/replication.py 100.00% <100.00%> (ø)
plugins/module_utils/implementations/mysql/user.py 100.00% <100.00%> (ø)
plugins/modules/mysql_replication.py 66.79% <100.00%> (-1.83%) ⬇️
...t/plugins/module_utils/test_mariadb_replication.py 100.00% <100.00%> (ø)
...s/module_utils/test_mariadb_user_implementation.py 100.00% <100.00%> (ø)
...nit/plugins/module_utils/test_mysql_replication.py 100.00% <100.00%> (ø)
... and 9 more

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 baea97d...23fedb7. Read the comment docs.

@Andersson007
Copy link
Collaborator

cc @bmalynovytch

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
Copy link
Collaborator

@Jorge-Rodriguez please resolve the conflict

@Jorge-Rodriguez Jorge-Rodriguez requested review from bmalynovytch and removed request for bmildren March 9, 2021 18:02
@Andersson007
Copy link
Collaborator

@Jorge-Rodriguez after you resolve the conflict, i think we could merge it. I emailed Benjamin 3 days ago. The tests passed, so i think it won't break anything.

@bmalynovytch
Copy link
Contributor

This looks really good, a part from the code duplication, but we can't do much about it 🙁

@bmalynovytch
Copy link
Contributor

I emailed Benjamin 3 days ago. The tests passed, so i think it won't break anything.

I know I'm a bit slow these days, no need to point it out 😝 (just kidding)

@Andersson007
Copy link
Collaborator

I emailed Benjamin 3 days ago. The tests passed, so i think it won't break anything.

I know I'm a bit slow these days, no need to point it out (just kidding)

Okay:) Anyway I didn't imply "Shame on @bmalynovytch":))))
It's just "he's informed", so, as in my country people sometimes say for fun, "silence is a sign of agreement":)))

@Jorge-Rodriguez Jorge-Rodriguez force-pushed the mysql_mariadb_dychotomy branch from 27bcf81 to 0ec8177 Compare March 11, 2021 18:10
@Jorge-Rodriguez
Copy link
Contributor Author

Rebased and fixed the conflict.
@Andersson007 do we need a documentation fragment for this?

@Andersson007
Copy link
Collaborator

@Jorge-Rodriguez ah, yes, we need (we don't need it for doc changes only), thanks for reminding:)

@Andersson007
Copy link
Collaborator

yeah, we must always remember about the fragments when going to merge someone's PR...

@Jorge-Rodriguez
Copy link
Contributor Author

yeah, we must always remember about the fragments when going to merge someone's PR...

We should add that to the wiki. Do you have the link to the relevant documentation from the developer guide? I always have trouble finding the article about changelog fragments

@Andersson007 Andersson007 merged commit 11958ec into ansible-collections:main Mar 16, 2021
@Andersson007
Copy link
Collaborator

@Jorge-Rodriguez thanks for the refactoring!
@bmalynovytch thanks for reviewing!

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.

3 participants