-
Notifications
You must be signed in to change notification settings - Fork 90
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
Adds support for CHANGE REPLICATION SOURCE TO
statement
#636
Adds support for CHANGE REPLICATION SOURCE TO
statement
#636
Conversation
@Andersson007 I kept the nomenclature so as not to significantly increase the list of parameters. |
elif mode == 'changereplication': | ||
chm = [] | ||
result = {} | ||
if primary_host is not None: |
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.
I don't like this similar repetition with changeprimary
mode. I can refactor if this make sense.
a8b7337
to
b210084
Compare
@dennisurtubia hello, thanks for the PR! Could you please add a changelog fragment and ping us when it's ready for review after the CI is green, thanks! |
@dennisurtubia yep, any meaningful refactoring is welcome but it'd be better to do it in a dedicated PR. And also feel free to add yourself to the |
f311e68
to
6405ddc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #636 +/- ##
==========================================
- Coverage 76.38% 75.34% -1.04%
==========================================
Files 19 18 -1
Lines 2456 2507 +51
Branches 618 640 +22
==========================================
+ Hits 1876 1889 +13
- Misses 403 420 +17
- Partials 177 198 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
changelogs/fragments/supports_mysql_change_replication_source_to.yml
Outdated
Show resolved
Hide resolved
@dennisurtubia LGTM, one small formatting thing ^ |
086f1b3
to
6fc2d85
Compare
@dennisurtubia thanks, LGTM @laurent-indermuehle any thoughts? |
I'm grumpy. First we had to rename master to primary and slave to replica. Almost done but there is still a lot of 'master' and 'slave' in many commands. Anyway, my only request about this PR is that the option is more clearly labeled "only for MySQL" please. |
6fc2d85
to
9028d2c
Compare
Hello, @laurent-indermuehle. I just updated the |
@laurent-indermuehle can we merge the PR now? |
Yes, Thanks @dennisurtubia having made the change I requested. I merge now! Thanks for your contribution! |
a80b805
into
ansible-collections:main
Hi @dennisurtubia, thanks for the PR. |
@betanummeric Ho good catch, thanks for the advice. query = 'START %s %s' % (term, ','.join(chm))
cursor.execute(query) |
@laurent-indermuehle Yes, this is unsafe as well. The |
@dennisurtubia could you fix this? if no time, can someone else do it? |
@betanummeric Then this whole file is unsafe: https://github.com/eryx12o45/community.mysql/blob/main/plugins/modules/mysql_replication.py Because we keep track of the executed queries in a list, how to make the cursor.execute safe? |
Hey @Andersson007. Yes, I will fix it this week |
@laurent-indermuehle I noticed it too. I used some existing code in I think it would be necessary to create an issue to resolve all issues. |
@dennisurtubia I've created the issue, please take a look |
SUMMARY
Adds new mode for
community.mysql.mysql_replication
module.This new mode adds support for MySQL
CHANGE SOURCE REPLICATION TO
statement.ISSUE TYPE
COMPONENT NAME
mysql_replication
ADDITIONAL INFORMATION
MySQL
CHANGE MASTER TO
statement was deprecated. DocI didn´t found a respective statement for MariaDB that replaces
CHANGE MASTER TO
. List of MariaDB commandsResolves #635