Skip to content
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

Update the list of supported connectors #178

Conversation

ziegenberg
Copy link
Contributor

@ziegenberg ziegenberg commented Jun 1, 2021

SUMMARY

Update the list of supported connectors (especially the links) in README.

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME
  • README.md
  • module_utils
ADDITIONAL INFORMATION

@ziegenberg
Copy link
Contributor Author

For a first try, I've gone with the current minimum version introduced in #163. So this would be:

  • PyMySQL >= v0.7.10
  • MySQLdb >= v1.2.5

@codecov
Copy link

codecov bot commented Jun 1, 2021

Codecov Report

Merging #178 (d510eeb) into main (71b2742) will decrease coverage by 0.79%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
- Coverage   76.92%   76.12%   -0.80%     
==========================================
  Files          20       20              
  Lines        1794     1801       +7     
  Branches      439      444       +5     
==========================================
- Hits         1380     1371       -9     
- Misses        268      281      +13     
- Partials      146      149       +3     
Impacted Files Coverage Δ
plugins/module_utils/mysql.py 70.00% <25.00%> (-13.14%) ⬇️
plugins/modules/mysql_query.py 86.58% <0.00%> (-2.44%) ⬇️
plugins/modules/mysql_db.py 74.21% <0.00%> (-0.35%) ⬇️

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 71b2742...d510eeb. Read the comment docs.

@Andersson007
Copy link
Collaborator

Andersson007 commented Jun 3, 2021

@ziegenberg thanks for the PR!

"msg": ".... Exception message: module 'MySQLdb' has no attribute '__version__'"

Is there another way to get it?

@Andersson007
Copy link
Collaborator

BTW before pushing, you can run all the tests locally, see https://github.com/ansible/community-docs/blob/main/create_pr_quick_start_guide.rst . It's very simple. Needs only Docker installed.

@Andersson007
Copy link
Collaborator

Andersson007 commented Jun 3, 2021

and for this collection you should use just --docker option without specifying a distro. So it'll run in a default container. My command usually look like (you should be in ~/ansible_collection/community/mysql/, the path is important, mysql is the cloned repo with changes, see the guide above):

ansible-test integration test_mysql_user --docker -vvv > ~/test.log

@Andersson007
Copy link
Collaborator

@ziegenberg if you find something unclear while reading the guide https://github.com/ansible/community-docs/blob/main/create_pr_quick_start_guide.rst. I'd be really happy if you share it (feel free to create an issue under https://github.com/ansible/community-docs).

@ziegenberg
Copy link
Contributor Author

@ziegenberg thanks for the PR!

"msg": ".... Exception message: module 'MySQLdb' has no attribute '__version__'"

Is there another way to get it?

Yeah, I know. What a bummer. But I haven't had enough time to looked into it further. It was a bit late the other day and I had no energy left to setup testing. So I thought let's see what ci will make of it. Looks like it wasn't too fond of my code. :)

I'll have to investigate into this issue and do some local debugging.

@Andersson007
Copy link
Collaborator

But I haven't had enough time to looked into it further. It was a bit late the other day and I had no energy left to setup testing.

There's no rush:) The main thing is not to burn out:)

@markuman
Copy link
Member

Maybe it MySQL-Python should be dropped and also replaced by mysqlclient like in ansible-collections/community.proxysql@df5ede0
plus rework error handling with from ansible.module_utils.basic import missing_required_lib

@Andersson007
Copy link
Collaborator

@ziegenberg @bmalynovytch @Jorge-Rodriguez what do you think of ^ ?

@Jorge-Rodriguez
Copy link
Contributor

@ziegenberg @bmalynovytch @Jorge-Rodriguez what do you think of ^ ?

I agree, but we shouldn't further to follow the usual procedure of warnings about the connector no longer being supported and so on

@Jorge-Rodriguez
Copy link
Contributor

@ziegenberg PR #163 has been superseded by #246

Copy link
Contributor

@Jorge-Rodriguez Jorge-Rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes should conform to the discussion in #141
Additionally, we should implement the replacement of Python-MySQLdb with mysqlclient, which will affect the whole collection.

At this point it'd be up to @ziegenberg whether he wants to implement that replacement here.

@ziegenberg
Copy link
Contributor Author

Hi!
Sorry for the long absence. I'll try to come up with the necessary changes in the next two weeks if that's ok with you.

@Jorge-Rodriguez
Copy link
Contributor

Hi! Sorry for the long absence. I'll try to come up with the necessary changes in the next two weeks if that's ok with you.

Welcome back! And thank you for working on this.

@Andersson007
Copy link
Collaborator

@ziegenberg feel free to re-open the PR or create a new one if you still want to finish it, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants