-
Notifications
You must be signed in to change notification settings - Fork 102
Add options to start and stop group replication #647
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
base: main
Are you sure you want to change the base?
Add options to start and stop group replication #647
Conversation
Andersson007
left a comment
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.
@eryx12o45 hello, thanks for the PR!
Do i understand it correctly that in our case it's tricky to cover this with integration tests? You tested it locally and it works OK, right?
Do others think if it's OK to merge the PR w/o integration testing?
laurent-indermuehle
left a comment
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.
The integration tests have 1 primary and 2 replicas. It should be possible to tests this no?
|
Hi @Andersson007 and @laurent-indermuehle , thanks for your review. I will try to create some tests in the near future. I created this PR in the first place some time back when I was working at another place where we would have needed it, but since then changes jobs and am not working on this anymore. Nevertheless I thought it makes sense to finalize this PR and therefore re-created it with adapted settings. But now I have to build a local test env. I would assume it is possible to test it though. I hope code-wise it's ok for now though |
@eryx12o45 hi, code-wise LGTM. Ideally would be great to have integration tests. If it's tricky (no idea), I'm OK with merging this provided the manual testing is done (though someone can break the code in future and we'll learn about it only via bug reports). |
|
@eryx12o45 please have a look at the Makefile. It uses Podman to run the integration tests on your workstation. If you have a question about it you can ask me (either here or in our matrix. I'm not a fan of incorporating code without proper tests. If down the line Oracle changes something to the group replication, due to the critical nature of the replication, I would like to know about it. |
|
Sorry @eryx12o45, I should have point you to this file: https://github.com/ansible-collections/community.mysql/blob/main/TESTING.md |
|
If there are any issue with running tests locally:
If you have not time at all, I don't know, just say it explicitly, maybe someone else could add testing directly to the PR |
|
Heyho, thanks for pointing me to the docs. As said. I will definitely try to biuild some integration tests, might just take some time, but I promise to try to keep it shorter this time ;-) |
c66568e to
dfefc76
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #647 +/- ##
==========================================
- Coverage 79.90% 76.54% -3.36%
==========================================
Files 32 20 -12
Lines 2871 2763 -108
Branches 719 704 -15
==========================================
- Hits 2294 2115 -179
- Misses 400 454 +54
- Partials 177 194 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@eryx12o45 hi, thanks for continuing to work on this! Please click "Ready for review" when it's ready so that it's not a draft anymore. Thanks |
|
Will do :-) I try to squeeze in som time whenever I can |
dac8e2b to
f89e238
Compare
|
@Andersson007 I tried my best to add test for everything, but the code coverage still says it's not tested. But I am not sure why. From my point of view it should be fine now. |
|
@eryx12o45 Green codecov checks are recommended, please ignore them and focus on making the other checks green |
01640cc to
54b4eff
Compare
|
@Andersson007 done :-) |
Andersson007
left a comment
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.
@eryx12o45 Thanks, I'll take a deeper look soon
| - result is changed | ||
| - result.queries | length > 0 | ||
| - "'START GROUP_REPLICATION' in result.queries[0]" | ||
| when: result is not failed |
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.
| when: result is not failed | |
| when: result is not failed |
i see many ignore_erros + this condition. The thing is that we can't be sure our test work.
can we handle it somehow w/o using ignore_errors + run if not failed ?
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.
@eryx12o45 ^ (besides the above, LGTM)
SUMMARY
Added options to start and stop group replication
ISSUE TYPE
Feature Pull Request
COMPONENT NAME
mysql_replication module
ADDITIONAL INFORMATION
new version of #93