-
Notifications
You must be signed in to change notification settings - Fork 89
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
Add TLS connection parameters #9
Add TLS connection parameters #9
Conversation
@Andersson007 The additional tests are still missing. I want to wait for the tests to pass with the existing code first. It seems the test suite is broken by the mysql_variables module |
|
there are a lot of errors actually. Strange why they didn't appear when all mysql stuff was moved to this repo recently |
@bmildren yes, starting from line 2531 of the log
The only changes that touched mysql_replication stuff were #7 , could you please take a look |
🤔 Hmm that's wierd, it looks like the CI started failing ~4 days ago, when the last commit was ~9 days ago. I can't think of something specific in the tests that might have changed elsewhere and could be affecting the result.. I'll have a dig and see what I can see. |
@Andersson007 that assertion error comes from a file that's not touched in this PR and it happens before my tests are run. |
yes, looks unrelated |
I've just tried to run mysql_replication tests locally by
|
The local error is just down to not having enough memory available for the 3 instances. |
added 4 GB to the test vm:)
|
Yep I've seen the same, I'm just trying to debug it now.. 👍 |
Looks like these are legit errors, currently we test using pymysql only (when time allows I plan to also add testing for mysqlclient-python), and the errors began when we automatically started pulling pymysql v0.10.0 as opposed to 0.9.3. v0.10.0 changes the behaviour of how warnings are handled (https://github.com/PyMySQL/PyMySQL/blob/master/CHANGELOG.md#v0100), and since MySQL 5.7 (we test on 5.7 & 8.0, but not 5.6), issuing START SLAVE when the replica is running results in a warning as opposed to an error. So with pymysql v0.10.0 the warning is no longer being reported. For now we can pin pymysql to 0.9.3, which will make the tests go green again, however we should look to address this with a more appropriate fix. |
@bmildren shall I add the pin on this PR or do I wait to rebase? |
Hey @Jorge-Rodriguez , sorry it caught me at a bad time today - it's out scope for this pr, so I'll push a quick fix, then you can rebase 🙂 👍 |
#10 is merged, feel free to rebase 👍 |
a18e5ae
to
b062f5f
Compare
Codecov Report
@@ Coverage Diff @@
## main #9 +/- ##
==========================================
+ Coverage 68.47% 68.91% +0.44%
==========================================
Files 8 8
Lines 1459 1528 +69
Branches 386 404 +18
==========================================
+ Hits 999 1053 +54
- Misses 315 325 +10
- Partials 145 150 +5
Continue to review full report at Codecov.
|
Ok, it seems my code has some unexpected side effects on MySQL 8. I'll see to fix that. The issue with codecov puzzles me though as the file with reduced coverage is not touched in this PR. |
@Andersson007 ok, the integration tests are green now and the new check mode tests are in place. I don't know what's the story with codecov though. |
@Jorge-Rodriguez thanks! |
@Andersson007 @bmildren It does seem that codecov does not like the Black formatting, which is a pity because we could use some consistency on the use of quotes. The line length fixes aren't too bad either if you don't mind the extra vertical space usage. |
8771fd6
to
b6e4216
Compare
Timeout on line 4096 of the failed integration tests logs. |
@Andersson007 does this look otherwise ok, or should I rebase/squash into a single commit? |
@Jorge-Rodriguez no, it's ok now, it'll be squashed during merge. |
@Jorge-Rodriguez i noticed some tests failed https://github.com/ansible-collections/community.mysql/pull/9/checks?check_run_id=911307917 |
I've just seen your comment. I triggered the tests. in community.general we can relaunch failed tests only, not here unfortunately:( |
lgtm 👍 |
There was a problem hiding this 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 contribution! |
@bmildren community.general has been recently released according to ansible-collections/community.general#582 |
yep cool that makes sense 👍 |
SUMMARY
Add support for TLS REQUIRES options, to enforce TLS client connections for users.
ISSUE TYPE
COMPONENT NAME
mysql_user
ADDITIONAL INFORMATION
Fixes #5