implement check_type and fix different errors

[markuman]

SUMMARY

closes: #19

• Implement check_type feature.
• fix different other issues
  • reader_hostgroup: 0
  • ability to change reader_hostgroup in general
  • add default value for comment. "" is used in source code

ISSUE TYPE

• Bugfix Pull Request
• Feature Pull Request

COMPONENT NAME

proxysql_replication_hostgroups

[Andersson007]

@markuman thanks for the new feature and the fixes!

My suggestions are

1. If the PR contains bugfixes, it would be good to add bugfixes: in the changelog fragment
2. in the future, it's better to add not tightly related things with separate PRs because a) it's much easier to review b) it's easier to revert changes later if needed
3. several cosmetic not necessary suggestions
4. idea: would be perfect to start checking actual system state with shell: or command: modules in integration tests in the future, i.e. to query the test DB via cli tools OR proxysql_query: module that we discussed should definitely help (maybe proxysql_info covers this case?).
5. it would be nice to have an example for the new option in the EXAMPLES block

Thank you!

[markuman]

@Andersson007

in the future, it's better to add not tightly related things with separate PRs because a) it's much easier to review b) it's easier to revert changes later if needed

Absolutely right. It just happen in a flow of coding.
I can split it if you like.

idea: would be perfect to start checking actual system state with shell: or command: modules in integration tests in the future, i.e. to query the test DB via cli tools OR proxysql_query: module that we discussed should definitely help (maybe proxysql_info covers this case?).

You mean, we did it now with shell/command and replace it with proxysql_query later?
I think it is also possible with proxysql_info now.

[Andersson007]

@markuman thanks for working on this!

• I think no need to split it now (as you prefer though)

You mean, we did it now with shell/command and replace it with proxysql_query later?
I think it is also possible with proxysql_info now.

Ah, yes, i greped through the tests/integration directory and i see them used.
Though I don't see them in the integration tests for this PR. Not sure if it's applicable. I personally prefer to check every change with another module. In postgres / mysql we have the*_query module which helps much.

[Andersson007]

if the check is not applicable here, feel free to skip my suggestion

[Andersson007]

LGTM

[markuman]

Just one small typo change bb59a10

[markuman]

And one left query style: f17af88

[Andersson007]

Thanks!

[Andersson007]

I'm going to checkout and run the tests locally in 5-10 minutes.

[markuman]

I'm going to checkout and run the tests locally in 5-10 minutes.

I will run now against various version, included 1.4.15

[Andersson007]

everything passed locally

[Andersson007]

LGTM