-
Notifications
You must be signed in to change notification settings - Fork 550
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
Add CentOS on Travis CI. #989
Conversation
@@ -135,15 +136,22 @@ def connect(*args) | |||
|
|||
# You may need to adjust the lines below to match your SSL certificate paths | |||
ssl_client = nil | |||
option_overrides = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This modification is to pass the test on the CentOS environment that does not have /etc/mysql/*.pem
files.
@@ -164,7 +164,7 @@ | |||
expect do | |||
res.each_with_index do |_, i| | |||
# Exhaust the first result packet then trigger a timeout | |||
sleep 2 if i > 0 && i % 1000 == 0 | |||
sleep 4 if i > 0 && i % 1000 == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This modification is to pass the test on CentOS.
spec/mysql2/client_spec.rb
Outdated
@@ -1,5 +1,6 @@ | |||
require 'spec_helper' | |||
|
|||
# rubocop:disable Metrics/BlockLength |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By below modification, the length of the block is more than the limitation.
As I was not sure how to fix, I added this ignored line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See file .rubocop_todo.yml.
But you may be right that the better approach is not to globally increase the limit, but to ignore it for this file and have a more sensible limit apply everywhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I will change current limit 850 to 860 to prevent below situation.
spec/mysql2/client_spec.rb:3:1: C: Metrics/BlockLength: Block has too many lines. [851/850]
@@ -73,6 +74,12 @@ matrix: | |||
addons: | |||
hosts: | |||
- mysql2gem.example.com | |||
- rvm: 2.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is a little bit tricky. Actually the tested Ruby on CentOS is not Ruby 2.4 but default installed Ruby on CentOS. But I was not sure how to update rvm: foo
to show rvm
is not used directly on Travis.
.travis_centos.sh
Outdated
sh .travis_setup_centos.sh | ||
|
||
# To avoid install error. | ||
sed -i '/eventmachine/d' Gemfile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? Can't we fix the install error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment.
It is to avoid install error by bundle install
on CentOS.
But let me check again to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason of the error is just because g++
command was not installed on the environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it and rebased.
32118f0
to
1c126cc
Compare
.travis.yml
Outdated
@@ -73,6 +74,12 @@ matrix: | |||
addons: | |||
hosts: | |||
- mysql2gem.example.com | |||
- rvm: 2.4 | |||
# Mark |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who is Mark?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Mark" is not people's name in this case. I want to describe env: DOCKER=centos
is just used to identify this test case from Travis test page. But I am going to remove the line.
.travis_Dockerfile_centos
Outdated
@@ -0,0 +1,6 @@ | |||
FROM centos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest to use tagged version with centos:7
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right. I will change it to centos:7
.
.travis_centos.sh
Outdated
set -eux | ||
|
||
# Install dependency packages and start mysqld service. | ||
sh .travis_setup_centos.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should go into the Dockerfile, so that you can benefit from layer caching of these dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right. Let me move the logic to the Dockefile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a problem to move the .travis_setup_centos.sh
to Dockerfile
RUN (that is executed on docker build
's timing).
We can not start mysqld daemon service in the process of docker build
. (Can not access the service).
So, I moved only the part to install dependency to the Dockerfile RUN part.
I added the modification as another commit to see the change easily for now.
How?
Thank you for the checking.
spec/mysql2/client_spec.rb
Outdated
@@ -1,5 +1,6 @@ | |||
require 'spec_helper' | |||
|
|||
# rubocop:disable Metrics/BlockLength |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See file .rubocop_todo.yml.
But you may be right that the better approach is not to globally increase the limit, but to ignore it for this file and have a more sensible limit apply everywhere else.
@sodabrew thank you for the review! I am sorry I did not noticed until now. I will check those later. |
9524d5a
to
d697180
Compare
@sodabrew I modified it! |
Looks good to me! Are you ready for merge, or still hacking on this PR? |
b3da64b
to
f72a894
Compare
@sodabrew I am ready for merge! I did squash the 2 commits to 1 commit now. |
Thank you! 😄 |
* upstream/master: Expose windows client authentication (brianmario#1018) Fix code snippet (brianmario#1002) Add CentOS on Travis CI. (brianmario#989) Bump version to 0.5.2 Travis apt-get update for MySQL 5.5 install Updating the mysql2_mysql_enc_to_rb conversion table to 8.0 List (brianmario#976) Add default-libmysqlclient-dev to the likely packages list Bump version to 0.5.1 Use the prepared statement performance schema if available (brianmario#960) README mysql2 0.5.x works with Rails 5.0.7, 5.1.6, and higher README be sure to read about the known limitations of prepared statements Add missing FREE_BINDS to prepared statement streaming error case (brianmario#958) Fix with --with-mysql-dir (brianmario#952) Prevent command out of sync errors with Prepared Statements (brianmario#958)
Related: #988 .