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

Added helper method to initialize connection from connection string #177

Merged
merged 3 commits into from Jun 14, 2018

Conversation

3 participants
@Cellane
Copy link
Contributor

Cellane commented May 31, 2018

This is an alternative implementation of PR #156, since I’m an idiot and looked for existing PRs on a wrong repository (vapor/fluent-mysql instead of vapor/mysql, the repo I ended up committing this fix to), it hopefully shouldn’t suffer from the issue #156 currently has.

A relevant test was added as well.

@Cellane

This comment has been minimized.

Copy link
Contributor Author

Cellane commented May 31, 2018

I’m not entirely too sure why the MySQL 8.0 test is failing but unfortunately, I can’t even simulate the situation locally as I seem unable to connect to a MySQL 8.0 Docker image from Vapor on my computer. If you can give me any pointers as to how to fix this, I’ll definitely try my best!

@tanner0101
Copy link
Member

tanner0101 left a comment

LGTM, don't worry about the 8.0 failing that is a separate issue 👍


let database = url.path

if database.hasPrefix("/") {

This comment has been minimized.

@Cellane

Cellane Jun 1, 2018

Author Contributor

Ah, I didn’t know about that method, fixed!

This comment has been minimized.

@Cellane

Cellane Jun 1, 2018

Author Contributor

… although now even the test on MySQL 5.5 is failing, but I don’t think it’s because of this change either.

@Cellane

This comment has been minimized.

Copy link
Contributor Author

Cellane commented Jun 4, 2018

After seeing a commit in the master branch that said tests are fixed I re-run the tests (I hope that’s fine) and now 5.5 works fine while 5.7 fails with a random number of failed tests, sometimes it’s 2, sometimes it’s just 1 😅

@tanner0101 tanner0101 changed the base branch from master to gm Jun 14, 2018

@tanner0101 tanner0101 added this to the 3.0.0 milestone Jun 14, 2018

@tanner0101 tanner0101 self-assigned this Jun 14, 2018

@tanner0101
Copy link
Member

tanner0101 left a comment

Thanks!

@tanner0101 tanner0101 merged commit fd83bfd into gm Jun 14, 2018

0 of 5 checks passed

ci/circleci: 5.7-linux Your tests are queued behind your running builds
Details
ci/circleci: 8.0-linux CircleCI is running your tests
Details
ci/circleci: 8.0-linux-fluent CircleCI is running your tests
Details
ci/circleci: linux-release CircleCI is running your tests
Details
ci/circleci: mariadb-10.3-linux CircleCI is running your tests
Details
@penny-coin

This comment has been minimized.

Copy link

penny-coin commented Jun 14, 2018

Hey @Cellane, you just merged a pull request, have a coin!

You now have 78 coins.

@tanner0101 tanner0101 deleted the init-from-connection-string branch Jun 14, 2018

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