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

Can't enable SSL with MariaDB driver library. (#1182) #1183

Merged
merged 4 commits into from
Jun 1, 2021

Conversation

vakuum
Copy link

@vakuum vakuum commented Apr 10, 2021

Please see #1182 for details.

@junaruga
Copy link
Contributor

Thanks for the PR.

Could you add new CI case for mysql-8.0 server and mariadb client?

@junaruga
Copy link
Contributor

@vakuum I tried to create the commit to create DB server: MySQL, DB client: MariaDB #1186 . You can pick up if you like.

I think the new CI case could create mysql2.so linking to the MariaDB client library. But I do not see new error. We might need to add a unit test to test this issue.

Run ldd lib/mysql2/mysql2.so
...
	libmariadb.so.3 => /usr/lib/x86_64-linux-gnu/libmariadb.so.3 (0x00007f73d1daf000)
...

@vakuum
Copy link
Author

vakuum commented Apr 11, 2021

@junaruga I added your commit 0f44a2e to the pull request.

@junaruga
Copy link
Contributor

junaruga commented Apr 11, 2021

@vakuum Thanks for the work! I am sorry. Could you remove the 2nd commit? I am sorry. Because I got a feedback that it is not certain if we support the new case of the DB server (MySQL) + DB client (MariaDB) by adding the CI case. #1186 (comment) .

And right now our Travis CI has MariaDB (server and client) cases. But the macro HAVE_CONST_MYSQL_OPT_SSL_ENFORCE is not set on the cases. I think that's way your error case is not captured by our current CI cases. #1182 (comment)

So, I think the right thing about the CI, is to update the one of the Travis MariaDB cases by enabling HAVE_CONST_MYSQL_OPT_SSL_ENFORCE macro. But right now I have no idea about how to add it.

@junaruga
Copy link
Contributor

Could you add my another commit to this PR? #1182 (comment) .

@@ -31,7 +31,7 @@ jobs:
- {os: ubuntu-16.04, ruby: 2.4, db: mariadb10.0, allow-failure: true}
# Comment out due to ci/setup.sh stucking.
# - {os: ubuntu-18.04, ruby: 2.4, db: mariadb10.1}
# `service mysql restart` fails.
# Allow failure due to the issue #965, #1165.
Copy link
Contributor

@junaruga junaruga Apr 16, 2021

Choose a reason for hiding this comment

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

I opened a PR #1193 to fix #965 and #1152 . After the PR will be merged, we can remove #965. There will be only #1165 .

vakuum and others added 3 commits April 16, 2021 17:30
Add tests to verify the following commmit.

Can't enable SSL with MariaDB driver library.
* Relax the matching condition.
  It's better to verify a warning by checking stderr.
  But for now, just relax the matching condition, due to complex conditions.

  ```
  expect do
    new_client(options)
  end.to_not output.to_stderr
  ```

* Change pending to skip in SSL tests.
  The skip method is right in this context.
  Because the pending method requires the test to fail.
  But in some cases the test passes.
@vakuum
Copy link
Author

vakuum commented May 27, 2021

@junaruga: Is there any plan to merge this PR?

@junaruga
Copy link
Contributor

junaruga commented May 27, 2021

@vakuum Sorry I do not have a permission to merge a PR on this repository. I am just a contributor on this repo.

@junaruga
Copy link
Contributor

junaruga commented May 27, 2021

@vakuum if you want a new mysql2 gem including this PR now, I can recommend a way to create a releasing branch including this PR's commits on your forked repository, then run gem build mysql2.spec to create the mysql2-<version>.gem to be used in a meantime.

@sodabrew
Copy link
Collaborator

sodabrew commented Jun 1, 2021

Sorry for the delay to review, this is a great PR, thank you @vakuum !

@sodabrew sodabrew merged commit 7f4e844 into brianmario:master Jun 1, 2021
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