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

Embed pymysql connector within the collection #643

Conversation

laurent-indermuehle
Copy link
Collaborator

@laurent-indermuehle laurent-indermuehle commented Jun 6, 2024

SUMMARY

This PR introduces significant changes to the MySQL connector integration. It's aimed at streamlining dependencies and ensuring consistent behavior by removing mysql_client support.

pymysql has a MIT license, who is compatible with the GPL-v3.

I believe this change will help us create better modules with better documented returned values and offer a simpler experience for our users. A significant portion of exceptions from our modules documentation could be removed.

Changes

  • Embed pymysql connector within the collection: This removes the need for external dependencies.
  • Remove custom test containers: Leverage the default containers from ansible-test for simplicity.
  • Expand test matrix: Add tests against Python 3.11 and 3.12.
  • Simplify codebase: Eliminate logic related to connector presence and version checks.

Impact on Users

If you are a user of mysql_client, please note that this change forces the usage of pymysql exclusively. Be aware that:

  • Behavioral Changes: The behavior of some tasks may change, as pymysql and other connectors sometimes produce different outputs for the same actions. The most notable difference could be task results showing as green instead of yellow or vice versa.

Pro

  • Reduced Dependencies: No need to install pymysql on each controlled node (no more pip install pymysql or dnf install pymysql).
  • Consistent Behavior: Ensures predictable behavior across all environments, eliminating confusion over which connector is used and for which version.
  • Simplified Testing: Fewer tests are needed, reducing maintenance overhead.

Cons

Thanks to @betanummeric, @Andersson007 and @markuman we also have a list of downside to look out for:

  • We need to release the collection each time PyMySQL issue a security fix. So the CI will need new tool to help us maintain the connector (dependabot?).
  • The collection may be slower because PyMySQL will need to be passed via SSH to the controlled node (up to 162Ko of Python)
  • Remove the choice of the connector from our users
  • Adds a breaking change

Request for Feedback

Given the scope of these changes, your feedback is crucial. Especially if you utilize mysql_client.

This change eliminates the need to install the connector on each
controlled node, as `pymysql` version 1.1.1 is now included. As a
result, we can safely assume its availability, thus simplifying the
testing process.

Also, I managed to remove the need for pre-built test containers. We
now use the default test containers from ansible-test.
Copy link

codecov bot commented Jun 7, 2024

Codecov Report

Attention: Patch coverage is 65.69837% with 862 lines in your changes missing coverage. Please review.

Project coverage is 66.23%. Comparing base (50e7413) to head (04af62c).

Current head 04af62c differs from pull request most recent head 6cef70b

Please upload reports for the commit 6cef70b to get more accurate results.

Files Patch % Lines
plugins/module_utils/pymysql/connections.py 48.45% 358 Missing and 75 partials ⚠️
plugins/module_utils/pymysql/cursors.py 44.72% 141 Missing and 11 partials ⚠️
plugins/module_utils/pymysql/_auth.py 27.14% 102 Missing ⚠️
plugins/module_utils/pymysql/converters.py 37.67% 89 Missing and 2 partials ⚠️
plugins/module_utils/pymysql/protocol.py 69.54% 45 Missing and 8 partials ⚠️
plugins/module_utils/pymysql/charset.py 93.44% 10 Missing and 2 partials ⚠️
plugins/module_utils/pymysql/__init__.py 73.80% 11 Missing ⚠️
plugins/module_utils/pymysql/times.py 75.00% 3 Missing ⚠️
plugins/module_utils/pymysql/err.py 93.75% 1 Missing and 1 partial ⚠️
plugins/module_utils/pymysql/optionfile.py 87.50% 1 Missing and 1 partial ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #643      +/-   ##
==========================================
- Coverage   75.26%   66.23%   -9.04%     
==========================================
  Files          18       31      +13     
  Lines        2515     4902    +2387     
  Branches      642      923     +281     
==========================================
+ Hits         1893     3247    +1354     
- Misses        425     1345     +920     
- Partials      197      310     +113     
Flag Coverage Δ
integration 66.19% <65.69%> (-8.32%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@laurent-indermuehle laurent-indermuehle marked this pull request as draft June 7, 2024 12:21
@laurent-indermuehle laurent-indermuehle changed the title Add tests python and connector present in rhel8 and rhel9 Embed pymysql connector within the collection Jun 7, 2024
@laurent-indermuehle
Copy link
Collaborator Author

@betanummeric, @rsicart, @Andersson007 and any other contributor. I would love your input on the idea of embedding the pymysql connector into the collection.

@betanummeric
Copy link
Member

Please don't make pymysql part of this repository. It should be installed separately, we don't need to become pymysql maintainers.

Also including pymysql would slow down module execution a bit because pymysql needs to be copied from the ansible controller to the target node. So far, pymysql is pre-installed on the target node and doesn't need to travel through the ansible ssh connection.

What if someone wants to use mysql_client? Why is pymysql better?

"Expand test matrix: Add tests against Python 3.11 and 3.12." <- please open separate PRs for independent changes

@laurent-indermuehle
Copy link
Collaborator Author

This PR as been rejected by almost all maintainers. The main reasons are security concerns (must release c.mysql everytime PyMySQL receive a security patch) and speed (Ansible must send the whole PyMySQL code through SSH each time a module is used in a Playbook).

I extracted the part were I removed the custom test containers and open PR #650.

Also, there is a vote to deprecate mysqlclient and support only PyMySQL in #645.

For the above reasons, I close this PR. Thank you all for your feedback.

@laurent-indermuehle laurent-indermuehle deleted the lie_add_tests_python_and_connector_present_in_rhel8_rhel9 branch June 20, 2024 11:21
@Andersson007
Copy link
Collaborator

It was a good and productive discussion here and on Matrix, thanks everyone!

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.

3 participants