add mysql_full_version and suffix return variable #115

rndmh3ro · 2021-03-11T19:59:03Z

SUMMARY

This PR adds two new variables: the suffix variable that is sometimes added to the version-string and the full_version-variable that contains the whole version string (to make it easier to compare versions).

Fixes #114

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

mysql_info

codecov · 2021-03-11T20:01:22Z

Codecov Report

Merging #115 (63595b9) into main (bd86e24) will increase coverage by 0.48%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #115      +/-   ##
==========================================
+ Coverage   75.75%   76.23%   +0.48%     
==========================================
  Files          12       13       +1     
  Lines        1712     1734      +22     
  Branches      438      440       +2     
==========================================
+ Hits         1297     1322      +25     
+ Misses        269      267       -2     
+ Partials      146      145       -1

Impacted Files	Coverage Δ
plugins/modules/mysql_info.py	`79.69% <100.00%> (+1.97%)`	⬆️
tests/unit/plugins/modules/test_mysql_info.py	`100.00% <100.00%> (ø)`

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 bd86e24...63595b9. Read the comment docs.

Andersson007

@rndmh3ro thanks for the PR
Please also add a changelog fragment https://docs.ansible.com/ansible/latest/community/development_process.html#creating-a-changelog-fragment

plugins/modules/mysql_info.py

Andersson007 · 2021-03-12T06:40:26Z

fyi: don't worry about codecov/project is red.
Waiting for the changelog fragment

rndmh3ro · 2021-03-12T06:48:08Z

I added the fragment!

changelogs/fragments/115-add_mysql_full_version_suffix_return_var.yml

Andersson007

LGTM
@Jorge-Rodriguez if the PR looks good to you too, feel free to merge

Jorge-Rodriguez · 2021-03-12T08:25:23Z

@rndmh3ro could you add an explicit integration test for the suffix output?
Currently we're testing only against MySQL do it should always be empty, and it'll help cover the use case when we finally at MariaDB integration testing.

Thanks.

plugins/modules/mysql_info.py

Andersson007 · 2021-03-12T08:42:29Z

plugins/modules/mysql_info.py

+      returned: if not excluded by filter
+      type: str
+      sample: "MariaDB"
+    full_version:


Suggested change

full_version:

full:

Andersson007 · 2021-03-12T08:42:45Z

plugins/modules/mysql_info.py

+            version = self.info['settings']['version'].split('.')
+
+            # full_version = "5.5.60-MariaDB"
+            full_version = self.info['settings']['version']


Suggested change

full_version = self.info['settings']['version']

full = self.info['settings']['version']

Andersson007 · 2021-03-12T08:42:58Z

plugins/modules/mysql_info.py

                release=int(release),
+                suffix=str(suffix),
+                full_version=str(full_version),


Suggested change

full_version=str(full_version),

full=str(full),

Andersson007 · 2021-03-12T08:43:10Z

tests/integration/targets/test_mysql_info/tasks/main.yml

@@ -48,7 +48,7 @@
    - assert:
        that:
        - result.changed == false
-        - result.version != {}
+        - "mysql_version in result.version.full_version"


Suggested change

- "mysql_version in result.version.full_version"

- "mysql_version in result.version.full"

Andersson007 · 2021-03-12T08:44:14Z

changelogs/fragments/115-add_mysql_full_version_suffix_return_var.yml

@@ -0,0 +1,2 @@
+minor_changes:
+- mysql_info - add `full_version` and `suffix` return values (https://github.com/ansible-collections/community.mysql/issues/114).


Suggested change

- mysql_info - add `full_version` and `suffix` return values (https://github.com/ansible-collections/community.mysql/issues/114).

- mysql_info - add `version.full` and `version.suffix` return values (https://github.com/ansible-collections/community.mysql/issues/114).

rndmh3ro · 2021-03-12T09:06:41Z

@Andersson007, thanks I'll add the changes locally.

Currently we're testing only against MySQL do it should always be empty, and it'll help cover the use case when we finally at MariaDB integration testing.

@Jorge-Rodriguez sadly it's not always empty. Looking at the CI-logs:

MySQL 8:

2021-03-12T08:50:26.7204419Z TASK [test_mysql_info : Collect info only about version and databases] *********
2021-03-12T08:50:27.1526986Z �[0;32mok: [testhost] => {"changed": false, "databases": {"information_schema": {"size": 0}, "mysql": {"size": 2654208}, "performance_schema": {"size": 0}, "sys": {"size": 16384}}, "version": {"full_version": "8.0.22", "major": 8, "minor": 0, "release": 22, "suffix": ""}}�[0m


2021-03-12T08:51:25.3928912Z TASK [test_mysql_replication : find out the database version] ******************
2021-03-12T08:51:25.9604764Z �[0;32mok: [testhost] => {"changed": false, "version": {"full_version": "8.0.22", "major": 8, "minor": 0, "release": 22, "suffix": ""}}�[0m

MySQL 5.7:

2021-03-12T08:49:45.3867627Z TASK [test_mysql_info : Collect info only about version and databases] *********
2021-03-12T08:49:45.8869227Z �[0;32mok: [testhost] => {"changed": false, "databases": {"information_schema": {"size": 163840}, "mysql": {"size": 2644595}, "performance_schema": {"size": 0}, "sys": {"size": 16384}}, "version": {"full_version": "5.7.31-log", "major": 5, "minor": 7, "release": 31, "suffix": "log"}}�[0m

2021-03-12T08:51:00.7500011Z TASK [test_mysql_replication : find out the database version] ******************
2021-03-12T08:51:01.4887762Z �[0;32mok: [testhost] => {"changed": false, "version": {"full_version": "5.7.31-log", "major": 5, "minor": 7, "release": 31, "suffix": "log"}}�[0m

How should we proceed here?

Jorge-Rodriguez · 2021-03-12T09:26:57Z

@rndmh3ro maybe a unit test would be better suited for the task

rndmh3ro · 2021-03-12T09:56:55Z

maybe a unit test would be better suited for the task

I'm not familiar with writing unit tests. My Python-knowledge in this regard is quite limited. How does one do that? :)

Jorge-Rodriguez · 2021-03-12T10:05:46Z

maybe a unit test would be better suited for the task

I'm not familiar with writing unit tests. My Python-knowledge in this regard is quite limited. How does one do that? :)

There are a couple of simple examples in the tests/units folder. If you still don't feel comfortable, just let me know and I can do it.

Andersson007 · 2021-03-12T10:08:17Z

@Jorge-Rodriguez it's a very good spot about unit tests.
If @rndmh3ro is not familiar with it, i wouldn't overwhelm him with it (but it's good to study how pytest work in general, just see tests/units/* content).
But if you like to figure out the topic, we could wait.
If not, i think i could implement the unit tests later (or anyone else if they wish), after this is merged if there's no concerns that the changes will break the module.
@Jorge-Rodriguez if you put possible input and output that we expect here, it would be really great.

What do you folks think?

(@rndmh3ro feel free to fix what i suggested for a while.)

Andersson007 · 2021-03-12T10:08:46Z

@Jorge-Rodriguez ah, didn't see your comment, cool, thanks!

rndmh3ro · 2021-03-12T10:18:16Z

I'd be glad if any of you two can write the unit test (and after that I'll try to understand it :)).

Andersson007 · 2021-03-12T10:31:25Z

@rndmh3ro sounds sensible

Jorge-Rodriguez · 2021-03-15T13:31:23Z

@rndmh3ro That unit test took a lot more magick than I expected. I'll be happy to go through it with you if you want.
@Andersson007 I included the changes you requested.

add changelog fragment

Co-authored-by: Andrew Klychkov <[email protected]>

rndmh3ro · 2021-03-15T19:44:12Z

I'll be happy to go through it with you if you want.

That'd be great. There are some parts I don't understand.
Either here or on IRC? On here it's probably better for discoverability so others could learn from it, too. However maybe this PR is not the right place for this.

Jorge-Rodriguez · 2021-03-16T05:16:33Z

@rndmh3ro I'll be hanging out as Tiriel on the Ansible channel on freenode

Andersson007

LGTM
@Jorge-Rodriguez thanks for the unit tests and adding the suggested changes!
Feel free to merge

Jorge-Rodriguez · 2021-03-16T07:49:24Z

Better yet would be to review the unit test in a repo discussion. @Andersson007 can we enable those?

Andersson007 · 2021-03-16T07:55:32Z

@Jorge-Rodriguez , enabled, please check

Andersson007 · 2021-03-16T08:05:19Z

@rndmh3ro there's more complex case in community.postresql. I'd add it myself there. I'll put a reference to this PR with a comment that the original idea is yours.

Jorge-Rodriguez · 2021-03-16T09:55:25Z

@Jorge-Rodriguez , enabled, please check

It works. Thanks

rndmh3ro · 2021-03-16T19:03:34Z

#122

Andersson007 reviewed Mar 12, 2021

View reviewed changes

plugins/modules/mysql_info.py Outdated Show resolved Hide resolved

Andersson007 reviewed Mar 12, 2021

View reviewed changes

changelogs/fragments/115-add_mysql_full_version_suffix_return_var.yml Outdated Show resolved Hide resolved

Andersson007 approved these changes Mar 12, 2021

View reviewed changes

Andersson007 reviewed Mar 12, 2021

View reviewed changes

plugins/modules/mysql_info.py Outdated Show resolved Hide resolved

Andersson007 reviewed Mar 12, 2021

View reviewed changes

Sebastian Gumprich and others added 5 commits March 15, 2021 15:58

add mysql_full_version and suffix return variable

61c652d

add changelog fragment

rephrase changelog fragment

dabf5c6

Update plugins/modules/mysql_info.py

b5f59ff

Co-authored-by: Andrew Klychkov <[email protected]>

Add changes as per PR review

20ce789

Add tests for new suffix output parameter

63595b9

Jorge-Rodriguez requested a review from Andersson007 March 15, 2021 16:14

Andersson007 approved these changes Mar 16, 2021

View reviewed changes

Jorge-Rodriguez merged commit a5ee4b3 into ansible-collections:main Mar 16, 2021

Andersson007 mentioned this pull request Mar 16, 2021

postgresql_info: add full, raw, and patch ret version values ansible-collections/community.postgresql#68

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add mysql_full_version and suffix return variable #115

add mysql_full_version and suffix return variable #115

rndmh3ro commented Mar 11, 2021

codecov bot commented Mar 11, 2021 •

edited

Loading

Andersson007 left a comment

Andersson007 commented Mar 12, 2021

rndmh3ro commented Mar 12, 2021

Andersson007 left a comment

Jorge-Rodriguez commented Mar 12, 2021

Andersson007 Mar 12, 2021

Andersson007 Mar 12, 2021

Andersson007 Mar 12, 2021 •

edited

Loading

Andersson007 Mar 12, 2021

Andersson007 Mar 12, 2021

rndmh3ro commented Mar 12, 2021

Jorge-Rodriguez commented Mar 12, 2021

rndmh3ro commented Mar 12, 2021

Jorge-Rodriguez commented Mar 12, 2021

Andersson007 commented Mar 12, 2021

Andersson007 commented Mar 12, 2021

rndmh3ro commented Mar 12, 2021

Andersson007 commented Mar 12, 2021

Jorge-Rodriguez commented Mar 15, 2021

rndmh3ro commented Mar 15, 2021

Jorge-Rodriguez commented Mar 16, 2021 •

edited

Loading

Andersson007 left a comment

Jorge-Rodriguez commented Mar 16, 2021

Andersson007 commented Mar 16, 2021

Andersson007 commented Mar 16, 2021

Jorge-Rodriguez commented Mar 16, 2021

rndmh3ro commented Mar 16, 2021

	full_version = self.info['settings']['version']
	full = self.info['settings']['version']

	- "mysql_version in result.version.full_version"
	- "mysql_version in result.version.full"

		@@ -0,0 +1,2 @@
		minor_changes:
		- mysql_info - add `full_version` and `suffix` return values (https://github.com/ansible-collections/community.mysql/issues/114).

	- mysql_info - add `full_version` and `suffix` return values (https://github.com/ansible-collections/community.mysql/issues/114).
	- mysql_info - add `version.full` and `version.suffix` return values (https://github.com/ansible-collections/community.mysql/issues/114).

add mysql_full_version and suffix return variable #115

add mysql_full_version and suffix return variable #115

Conversation

rndmh3ro commented Mar 11, 2021

SUMMARY

ISSUE TYPE

COMPONENT NAME

codecov bot commented Mar 11, 2021 • edited Loading

Codecov Report

Andersson007 left a comment

Choose a reason for hiding this comment

Andersson007 commented Mar 12, 2021

rndmh3ro commented Mar 12, 2021

Andersson007 left a comment

Choose a reason for hiding this comment

Jorge-Rodriguez commented Mar 12, 2021

Andersson007 Mar 12, 2021

Choose a reason for hiding this comment

Andersson007 Mar 12, 2021

Choose a reason for hiding this comment

Andersson007 Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Andersson007 Mar 12, 2021

Choose a reason for hiding this comment

Andersson007 Mar 12, 2021

Choose a reason for hiding this comment

rndmh3ro commented Mar 12, 2021

Jorge-Rodriguez commented Mar 12, 2021

rndmh3ro commented Mar 12, 2021

Jorge-Rodriguez commented Mar 12, 2021

Andersson007 commented Mar 12, 2021

Andersson007 commented Mar 12, 2021

rndmh3ro commented Mar 12, 2021

Andersson007 commented Mar 12, 2021

Jorge-Rodriguez commented Mar 15, 2021

rndmh3ro commented Mar 15, 2021

Jorge-Rodriguez commented Mar 16, 2021 • edited Loading

Andersson007 left a comment

Choose a reason for hiding this comment

Jorge-Rodriguez commented Mar 16, 2021

Andersson007 commented Mar 16, 2021

Andersson007 commented Mar 16, 2021

Jorge-Rodriguez commented Mar 16, 2021

rndmh3ro commented Mar 16, 2021

codecov bot commented Mar 11, 2021 •

edited

Loading

Andersson007 Mar 12, 2021 •

edited

Loading

Jorge-Rodriguez commented Mar 16, 2021 •

edited

Loading