Adding an ability to set a certain check_type when setting up MySQL replication host groups

[zentavr]

SUMMARY

Adds a feature described at #19

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

proxysql_replication_hostgroup

[codecov]

Codecov Report

Merging #20 (b4c48bd) into main (2438c0f) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #20      +/-   ##
==========================================
+ Coverage   70.02%   70.05%   +0.03%     
==========================================
  Files           8        8              
  Lines         994      995       +1     
  Branches      164      164              
==========================================
+ Hits          696      697       +1     
  Misses        222      222              
  Partials       76       76              

Impacted Files	Coverage Δ
plugins/modules/proxysql_replication_hostgroups.py	73.07% <100.00%> (+0.20%)	⬆️

---

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 2438c0f...b4c48bd. Read the comment docs.

[felixfontein]

I don't know proxysql so I cannot say much about your change except some formal stuff :) You definitely need a changelog fragment.

[Andersson007]

@zentavr thanks for the contribution!

In addition to what @felixfontein mentioned, could you please cover the option in CI tests, see https://github.com/ansible-collections/community.proxysql/tree/main/tests/integration/targets

[felixfontein]

@zentavr could you please remove the @ mentions from the commit messages? Mentioning people in commit messages causes a flood of notifications for the mentioned persons (every time this commit is pushed to some repo), which can get pretty annoying :)

[zentavr]

@felixfontein - I had removed the mentions.

[felixfontein]

@zentavr thanks! :)

[zentavr]

@Andersson007 - added CI tests

[Andersson007]

@zentavr thanks for the tests!
I'm not a proxysql user, so can't judge anything except general things like doc formatting and tests.
We should get feedback from users.
Also a couple of general questions:

1. is this feature common for all proxysql versions? I mean, if it's been introduced recently, for example, won't the changes somehow break current users playbooks using the older proxysql versions / driver versions?
2. is it possible to add a case with check_mode: yes to the tests?

[zentavr]

@Andersson007 here I had found that check_type appeared since 2.x version of ProxySQL. Saying, the release date of 2.0.2 is 2019-02-18 (this is what I had found in ProxySQL Repo).

As for your second question: As I understood you correctly, you do want to have tests with check_type option and without it - correct?

[zentavr]

The tests of the module rely on 2.0.12 version of ProxySQL which has check_type. At the early beginning there were only 3 possible values to specify for the check_type.

[Andersson007]

@zentavr

As for your second question: As I understood you correctly, you do want to have tests with check_type option and without it - correct?

Not really, i meant running in check mode, i.e., passing check_mode: yes as an option (because the module supports the check mode, so better to cover this case too)

@Andersson007 here I had found that check_type appeared since 2.x version of ProxySQL. Saying, the release date of 2.0.2 is 2019-02-18 (this is what I had found in ProxySQL Repo).

So, will these changes break users playbooks if they use ProxySQL versions of lower than 2.0.2?
If yes:

1. We should mention in the option's description that this feature is supported since ProxySQL 2.0.2, otherwise ignored
2. Get and check the version in the code: if 2.0.2 and higher, append the option, else, pass. You can use this PR mysql_replication: fix crashes caused by deprecated terminology community.mysql#71 as an example (changes in mysql_replication.py and the unit tests)

[Andersson007]

any updates on this?

[zentavr]

@Andersson007 Currently very busy with my regular work, so I had no chance to check up your comments.

[Andersson007]

@zentavr if you're busy what about if anyone else complete it, what do you think?
@bmildren as the modules author could you please take a look?

[zentavr]

@Andersson007 I’m ok with your proposal.
Seems like the tests is the only open question

[felixfontein]

Please do NOT modify changelogs/changelog.yaml directly. Create a changelog fragment.

[Andersson007]

@zentavr as the only contributor to the collection so far, do you have any ideas if the PR #25 brings value? Maybe you work with the module.

[zentavr]

Hello @Andersson007. We do not use query_rules so heavy (we've implemented ProxySQL few month ago into our infrastructure), but the proposed features sound very interesting.

[Andersson007]

Hello @zentavr , cool:) Thanks for the feedback!

[markuman]

I'll take a look.
In the past I was failing to implement it, because of breaking backwards compatibility and the fast-moving and changing proxysql itself.

[markuman]

Yeah, it's the same issue. The check_type schema was introduced with 2.0.1.
All users that are using 2.0.0 or the 1.4.x releases won't be able to use this module anymore.
Imho, backwards compatibility must granted. Dropping the support for version < 2.0.1 must be first throw some deprecation warnings for some time.

[zentavr]

@markuman actually if they won't specify the check_type - nothing should happen.

[markuman]

@markuman actually if they won't specify the check_type - nothing should happen.

It don't looks like, because check_type comes with a default value read_only and the queries are all extended with that column.

[zentavr]

@markuman I see. What could be the workaround?

from one side we could try to stick to the very default functionality and be compatible with old and new versions of ProxySQL.

another solution could be a ProxySQL version checking before we start any SQL questions and being silent or throwing the exception of the user specified the parameter which is not supported by the ProxySQL version.

[Andersson007]

Checking version sounds like a good solution. We do it in community.mysql:

./plugins/module_utils/mysql.py get_server_version function
./plugins/module_utils/implementations/mysql/replication.py 
./plugins/module_utils/implementations/mysql/user.py

[markuman]

@markuman I see. What could be the workaround?

@zentavr

modify the sql query for every version results in many if/else statements.The code gets confusing. That was what I was trying in the past.

From today's perspective I would try to leave the "very default functionality" as it is.
And when check_type is requested and the underlying proxysql version supports it, I would introduce update statements, that'll handle only the check_type column - in a new function that is separated from the "very default functionality".
That keeps the code clean, maintanable and can be a path for further proxysql version-based features/parameters and help to keep backward compatibility.

[markuman]

Implemented via #69

[Andersson007]

thanks you all folks!