-
Notifications
You must be signed in to change notification settings - Fork 87
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
mysql_replication: deprecation of slave related options, adding alternatives #97
mysql_replication: deprecation of slave related options, adding alternatives #97
Conversation
Codecov Report
@@ Coverage Diff @@
## main #97 +/- ##
==========================================
- Coverage 75.07% 74.75% -0.32%
==========================================
Files 12 12
Lines 1645 1660 +15
Branches 423 430 +7
==========================================
+ Hits 1235 1241 +6
- Misses 267 272 +5
- Partials 143 147 +4
Continue to review full report at Codecov.
|
@bmalynovytch @Jorge-Rodriguez @felixfontein @steveteahan There's a big question: should we change the messages like |
I think we should go for the change and use inclusive language. Making a breaking change shouldn't make us stick with derogatory terms. That being said, it is still a breaking change, so I think we should make great fanfare about it and begin announcing it even before we PR the changes. |
@bmalynovytch @Jorge-Rodriguez @felixfontein @steveteahan |
@bmalynovytch @Jorge-Rodriguez @steveteahan ready for review |
plugins/modules/mysql_replication.py
Outdated
@@ -490,8 +506,14 @@ def main(): | |||
# "REPLICA" must be used instead of "SLAVE" | |||
if uses_replica_terminology(cursor): | |||
replica_term = 'REPLICA' | |||
if master_use_gtid == 'slave_pos': | |||
module.warn('master_use_gtid "slave_pos" value is deprecated. ' |
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.
Why not use module.deprecate()
and disallow slave_pos
in the major release?
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.
@felixfontein 1) I wouldn't like to deprecate it in observable future 2) it's hard to predict the exact version 3) I'm waiting for anybody persuades me that it's worth to deprecate and disallow eventually :)
What do you folks think?
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.
@felixfontein do you mean exactly slave_pos
or all the expressions?
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.
@Andersson007 I mean for all of them :)
What do you mean by "it's hard to predict the exact version"?
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.
@felixfontein I wan't clear sorry, I mean if we say it'll be community.mysql 2.0 or 3.0 or whatever, I'm not sure it will happen. This collection is too small and breaking changes aren't introduced regularly, so it's hard to choose version and to correlate it with a term.
Let's think.. I think we could:
- change releasing policy to release from special branches (now we release from
main
) - introduce only change of the messages I mentioned in the fragment in the next major release (2.0.0), say in 3-6 months
- and then remove the options I added the aliases to in 3.0.0 in 6-12 months since now
What do you folks think?
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 think the process makes sense, although I don't quite understand the impact of releasing from special branches.
Also, in a shameless wedge to push my own agenda, I think the process applies as well to the issue outlined in #101
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.
@Jorge-Rodriguez briefly it's more complicated than just to release from main. We'll have to backport things to release branches, e.g. to stable-1
, release fix versions for all supported branches, etc. Anyway, we have no option with deprecated
terminology - it must be replaced, so will release from branches, no problem.
@Andersson007 @felixfontein @bmalynovytch @steveteahan |
@Jorge-Rodriguez If there's a dedicated issue, it would be really good:) So we can discuss the proposal there |
You're very right, see #101 |
@felixfontein changed I think we could:
@felixfontein @Jorge-Rodriguez @bmalynovytch what do you think? |
@Andersson007 sounds reasonable. I'd align the major version release dates with Ansible release dates if possible. (Ansible 4.0.0 is somewhat close, but 5.0.0 will probably be in a bit more than 6 months, and 6.0.0 in a bit more than a year.) |
@felixfontein the aligning sounds good, thanks. A year is a good term, I think. Everybody should get annoyed with the warnings and switch to the "good" options:) |
If nobody disagrees, I'll change the release issue next week, will do it similarly to community.general if there's no better option |
@Jorge-Rodriguez @felixfontein thanks for reviewing! |
(@Jorge-Rodriguez I don't think that anybody else will review the PR, so you, as a committer, reviewed and approved it, that's why I merged it myself) |
@Andersson007 while working on something else, I've spotted instances of the deprecated language in the |
@Jorge-Rodriguez good spot, thanks! I'll create an issue not to forget. If you like to do it, please, do. If no time / wish, let me know. |
SUMMARY
mysql_replication: deprecation of slave related options, adding alternatives
also announce that the word SLAVE will be replaced with REPLICA in the next major release
Relates to #78 and #98
ISSUE TYPE
COMPONENT NAME
mysql_replication