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

MySQL 8 Compatibility and SHA256 authentication plugin support #312

Merged
merged 5 commits into from Jul 9, 2018
Merged

MySQL 8 Compatibility and SHA256 authentication plugin support #312

merged 5 commits into from Jul 9, 2018

Conversation

terrycain
Copy link
Collaborator

Fixes #302
Fixes #297

This PR contains the bulk of what PyMySQL 0.9.0 added which was support for MySQL 8+'s use of the sha256_password and caching_sha2_password authentication plugin.

I've also parameterised mysql_tag so it now tests connecting to MySQL 5.6 and 8.0 using SSL and as MySQL 8 defaults to the caching_sha2_password, that is tested by the current SSL unit tests.

I'm planning on creating a fixture that allows me to skip tests depending on the value of mysql_tag so I can target a few specific tests to 8.0 which should further test the sha256_password plugin.


@jettify I'm assuming you have no issue with me making a PR to move the other tests to using the MySQL server provided by the container, that way I think we can get rid of all the travis matrix except PYTHONASYNCIODEBUG.

In the top of conftest.py were checking for version 3.5 and optionally enabling uvloop, seeing as we've dropped less than 3.5.3 support that can go.

@terrycain terrycain self-assigned this Jul 8, 2018
@terrycain terrycain requested a review from jettify July 8, 2018 19:37
@codecov
Copy link

codecov bot commented Jul 8, 2018

Codecov Report

Merging #312 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #312   +/-   ##
=======================================
  Coverage   92.85%   92.85%           
=======================================
  Files           9        9           
  Lines        1119     1119           
  Branches      161      161           
=======================================
  Hits         1039     1039           
  Misses         56       56           
  Partials       24       24

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 4897f50...332a249. Read the comment docs.

else:

if plugin_name == b"mysql_native_password":
Copy link
Member

Choose a reason for hiding this comment

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

probably good idea move this block left and change else to elif

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Issue is we dont want to execute lines 831-833 when doing the sha256 plugins

@terrycain
Copy link
Collaborator Author

It seems theres some weirdness/bug with pytest, fixture scoping and multiple sets of parameterisation... that or I couldn't find the magical combination to cause pytest not to spin up containers when it didn't need to. So I unrolled the parameterisation on test_sha_connection as that saves around 200 seconds.

Pytest possibly has some bug in the fixture scoping when theres multiple sets of parameterisation
Unrolling the test parameterisation for SHA meant less containers spun up and down.
)

# magic numbers:
# 2 - request public key
Copy link
Member

Choose a reason for hiding this comment

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

Good idea to make those numbers into enum, but I am fine merging PR as it is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd agree, but leaving them as they are does give us the benefit that it looks near identical to the equivalent pymysql function.

@terrycain terrycain merged commit 884802c into aio-libs:master Jul 9, 2018
@terrycain
Copy link
Collaborator Author

@jettify how do you go about doing a release to pypi?

@jettify
Copy link
Member

jettify commented Jul 9, 2018

lets do this!

@jettify
Copy link
Member

jettify commented Jul 9, 2018

Just create release https://github.com/aio-libs/aiomysql/releases here

@terrycain
Copy link
Collaborator Author

Cool we now have v0.0.18

@LemonPi
Copy link

LemonPi commented Jul 10, 2018

Will test it out as soon as pypi updates their index (tomorrow?)
It still shows 0.0.17 as the latest.

P.S. what features will 0.1 have and what about 1.0 :P?

@terrycain
Copy link
Collaborator Author

@LemonPi A while ago we were thinking just jumping to 1.0.0 after we cleaned up the code a bit

@jettify
Copy link
Member

jettify commented Jul 10, 2018

@terrycain 0.0.18 deploy failed https://travis-ci.com/aio-libs/aiomysql/builds/78487908, you need also bump version in __init__.py, because PyPI does not allow to upload the same version.

I suggest bump version to 0.0.19 and create a new release.

@jettify
Copy link
Member

jettify commented Jul 10, 2018

Or even maybe 0.1.0 version

@terrycain
Copy link
Collaborator Author

Ahh of course :D. Will bump 0.1.0 later today and rerelease

@terrycain
Copy link
Collaborator Author

Bumped version to 0.0.19 as I forgot about 0.1.0 when I was doing it :D We'll do 0.1.0 next time

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

Successfully merging this pull request may close these issues.

Compatibility with PyMySQL == 0.9.0 Cannot connect to mysql 8
3 participants