Using show all slaves status when using MariaDB to be consistent with MySQL
#602
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #602 +/- ##
==========================================
- Coverage 76.66% 74.86% -1.81%
==========================================
Files 28 18 -10
Lines 2447 2327 -120
Branches 603 585 -18
==========================================
- Hits 1876 1742 -134
- Misses 388 405 +17
+ Partials 183 180 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@wfelipew thanks for the issue and the PR!
@wfelipew @laurent-indermuehle i see you folks are still discussing some implementation things. I'll be happy with everything you'll agree on as I'm not a user.
This is a kind reminder that we need to add a changelog fragment here. @wfelipew could you please add it?
Thanks
|
Hey @Andersson007 just added the changelog fragment, could you take a look? |
plugins/modules/mysql_info.py
Outdated
| def is_mariadb(self): | ||
| if self.server_implementation == "mariadb": | ||
| return True | ||
| else: | ||
| return False | ||
|
|
||
| def is_mysql(self): | ||
| if self.server_implementation == "mysql": | ||
| return True |
There was a problem hiding this comment.
This would be more at home in module_utils/mysql.py
And it's crazy to think we have a variable db_engine in the integrations tests but we don't have an equivalent in the modules. If we implement this in Python, we could then use it in the integrations tests.
There was a problem hiding this comment.
Just moved it to the module_utils/mysql.py
plugins/modules/mysql_info.py
Outdated
| cursor.execute("SELECT VERSION()") | ||
| if 'mariadb' in cursor.fetchone()["VERSION()"].lower(): | ||
| server_implementation = "mariadb" | ||
| else: | ||
| server_implementation = "mysql" | ||
|
|
There was a problem hiding this comment.
This could use get_server_version() from module_utils/mysql.py.
There was a problem hiding this comment.
Just moved it to the module_utils/mysql.py and using the get_server_version() instead of running the SELECT VERSION()
Co-authored-by: Laurent Indermühle <[email protected]>
…ql into show-all-replicas
|
@Andersson007 It's a bit late to fire up my brain, but don't you think the module_utils @wfelipew is implementing to differentiate the behavior between MySQL and MariaDb deserve it's own PR? Also, rather than |
@laurent-indermuehle SGTM, how about implementing the refactoring in a separate PR? |
Co-authored-by: Andrew Klychkov <[email protected]>
plugins/module_utils/mysql.py
Outdated
|
|
||
|
|
||
| def is_mariadb(implementation): | ||
| if implementation == "mariadb": | ||
| return True | ||
| else: | ||
| return False | ||
|
|
||
|
|
||
| def is_mysql(implementation): | ||
| if implementation == "mysql": | ||
| return True | ||
| else: | ||
| return False |
There was a problem hiding this comment.
| def is_mariadb(implementation): | |
| if implementation == "mariadb": | |
| return True | |
| else: | |
| return False | |
| def is_mysql(implementation): | |
| if implementation == "mysql": | |
| return True | |
| else: | |
| return False |
plugins/modules/mysql_info.py
Outdated
| @@ -292,6 +293,8 @@ | |||
| mysql_driver_fail_msg, | |||
| get_connector_name, | |||
| get_connector_version, | |||
| get_server_implementation, | |||
| is_mariadb, | |||
There was a problem hiding this comment.
| is_mariadb, |
plugins/modules/mysql_info.py
Outdated
| @@ -497,7 +501,10 @@ def __get_master_status(self): | |||
|
|
|||
| def __get_slave_status(self): | |||
| """Get slave status if the instance is a slave.""" | |||
| res = self.__exec_sql('SHOW SLAVE STATUS') | |||
| if is_mariadb(self.server_implementation): | |||
There was a problem hiding this comment.
| if is_mariadb(self.server_implementation): | |
| if self.server_implementation == "mariadb": |
| __metaclass__ = type | ||
|
|
||
| import pytest | ||
|
|
||
| from ansible_collections.community.mysql.plugins.module_utils.mysql import get_server_version | ||
| from ansible_collections.community.mysql.plugins.module_utils.mysql import get_server_version, get_server_implementation, is_mariadb, is_mysql |
There was a problem hiding this comment.
| from ansible_collections.community.mysql.plugins.module_utils.mysql import get_server_version, get_server_implementation, is_mariadb, is_mysql | |
| from ansible_collections.community.mysql.plugins.module_utils.mysql import get_server_version, get_server_implementation |
|
|
||
| def test_is_mysql(): | ||
| """ | ||
| Test that server is_mysql return expect results | ||
| """ | ||
| assert is_mysql("mysql") is True | ||
| assert is_mysql("mariadb") is False | ||
|
|
||
|
|
||
| def test_is_mariadb(): | ||
| """ | ||
| Test that server is_mariadb return expect results | ||
| """ | ||
| assert is_mariadb("mariadb") is True | ||
| assert is_mariadb("mysql") is False |
There was a problem hiding this comment.
| def test_is_mysql(): | |
| """ | |
| Test that server is_mysql return expect results | |
| """ | |
| assert is_mysql("mysql") is True | |
| assert is_mysql("mariadb") is False | |
| def test_is_mariadb(): | |
| """ | |
| Test that server is_mariadb return expect results | |
| """ | |
| assert is_mariadb("mariadb") is True | |
| assert is_mariadb("mysql") is False |
|
@laurent-indermuehle, just removed the |
|
As @laurent-indermuehle is busy now, I'm merging the PR. @wfelipew thanks for the contribution! Any other from you will be much appreciated! @wfelipew if you're not already there, we have a Matrix channel and a forum group. All details are in the communication guide, fyi and welcome to join |
Andersson007
merged commit 852c19a
into
ansible-collections:main
SUMMARY
Using
show all slaves statuswhen using MariaDB to be consistent with MySQL behaviourAccording to MySQL documentation if we run
SHOW SLAVE STATUS(without a specific replication channel) it should return all replication channels.But In MariaDB, if we want to get all replication channels we need to use the
SHOW ALL SLAVES STATUSotherwise it will only return the default channel.Fixes #603
ISSUE TYPE
COMPONENT NAME
mysql_info
ADDITIONAL INFORMATION