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

Add GitHub Actions. #1154

Merged
merged 1 commit into from
Feb 23, 2021
Merged

Conversation

junaruga
Copy link
Contributor

@junaruga junaruga commented Jan 11, 2021

This is a PR to start GitHub Actions. It's a minimal start to have the compatible or covered cases on GitHub Actions from Travis eventually. This is my first experience to work on GitHub Actions.

I find GitHub Actions specific limitations.

  • I am using https://github.com/ruby/setup-ruby common library to install Ruby and Bundler, and bundle install that is commonly used in Ruby applications. However while the current setup needs to run the bundle install --without benchmarks development on Travis, I could not find the way to inject it to the common library on GitHub Actions. So, I modified Gemfile to update the groups to be optional as bundle install worked. You can see the detail here. Without the modification, the bundle install fails on the environment.
  • According to the GitHub Actions available ubuntu environment, there are only 3 available environments, ubuntu-20.04 (focal), ubuntu-18.04 (bionic), ubuntu-16.04 (xenial), while the trusty and xenial are used in Travis. See the Ubuntu version history.

I saw the following error on the cases on GitHub. Do you have any idea to fix this? This blocks the PR.

$ bash .travis_setup.sh
...
 + mysql -u root -e 'CREATE DATABASE /*M!50701 IF NOT EXISTS */ test'
ERROR 1045 (28000): Access denied for user 'root'@'localhost' (using password: NO)
Error: Process completed with exit code 1.

Thanks.

@junaruga
Copy link
Contributor Author

I saw the following error on the cases on GitHub. Do you have any idea to fix this? This blocks the PR.

Here is the error log on my forked repository.
https://github.com/junaruga/mysql2/runs/1683056833?check_suite_focus=true#step:5:32

@junaruga junaruga mentioned this pull request Jan 11, 2021
@junaruga junaruga force-pushed the wip/ci-github-actions branch from 316a771 to 94a3d5b Compare January 11, 2021 19:07
@junaruga
Copy link
Contributor Author

junaruga commented Jan 11, 2021

Here is the error log on my forked repository.
https://github.com/junaruga/mysql2/runs/1683056833?check_suite_focus=true#step:5:32

It seems there is no mysql 5.6 packages such as mysql-server-5.6 on ubuntu-16.04 xenial. But there is mysql 5.7 packages on it.

Running the following commands with the modification, I see those packages are already installed.

--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -17,6 +17,7 @@ jobs:
       # if any matrix job fails unlike Travis fast_finish.
       fail-fast: false
     steps:
+      - run: sudo apt-get -yq install mysql-server-5.7 mysql-client-core-5.7 mysql-client-5.7
       - uses: actions/checkout@v2
       # https://github.com/ruby/setup-ruby
       - uses: ruby/setup-ruby@v1
$ sudo apt-get -yq install mysql-server-5.7 mysql-client-core-5.7 mysql-client-5.7
...
Reading state information...
mysql-client-5.7 is already the newest version (5.7.32-0ubuntu0.16.04.1).
mysql-client-5.7 set to manually installed.
mysql-client-core-5.7 is already the newest version (5.7.32-0ubuntu0.16.04.1).
mysql-client-core-5.7 set to manually installed.
mysql-server-5.7 is already the newest version (5.7.32-0ubuntu0.16.04.1).
mysql-server-5.7 set to manually installed.
0 upgraded, 0 newly installed, 0 to remove and 9 not upgraded.

And I see the following error about the password is on the mysql 5.7.

+ mysqld --version
mysqld  Ver 5.7.32-0ubuntu0.16.04.1 for Linux on x86_64 ((Ubuntu))
+ mysql -u root -e 'CREATE DATABASE /*M!50701 IF NOT EXISTS */ test'
ERROR 1045 (28000): Access denied for user 'root'@'localhost' (using password: NO)
Error: Process completed with exit code 1.

In .travis_setup.sh, the used package libmariadbclient-dev is not avalable on ubuntu-16.04 xenial. We need to find an alternative libmariadbclient-dev package to be used in xmariadb10.0 and xmariadb10.1 cases.

@sodabrew
Copy link
Collaborator

This is great! Thank you! The .travis_mysql57 / .travis_mysql80.sh scripts set the root password and should work for this.

@sodabrew
Copy link
Collaborator

I think it would be reasonable to move all the tests up to Ubuntu 20.04. The most common problem I'm seeing in the issues tracker these days are out of date Rails versions on up to date MySQL versions. This is unexpected, in my experience I've always updated my application code long before the database engine!

@junaruga
Copy link
Contributor Author

I think it would be reasonable to move all the tests up to Ubuntu 20.04.

I can agree on it. But there might be older mysql/mariadb versions that are not available on Ubuntu 20.04 but available on Ubuntu 18.04 or Ubuntu-16.04. So, I think we can "basically" use Ubuntu 20.04 for the cases as much as possible, but if the tested mysql/mariadb version's packages do not exist on Ubuntu 20.04, we can try to use another old Ubuntu environment.

Thanks for the info to fix the issues. Tomorrow I will take a look at it!

@junaruga junaruga force-pushed the wip/ci-github-actions branch from 94a3d5b to 77677ad Compare January 12, 2021 17:48
@junaruga
Copy link
Contributor Author

Rebased this PR.

The .travis_mysql57 / .travis_mysql80.sh scripts set the root password and should work for this.

I still see the error by the SQL command to set the root password. Do you have any idea to fix it?

https://github.com/junaruga/mysql2/runs/1689931827?check_suite_focus=true#step:5:23

+ mysql -u root -e 'ALTER USER '\''root'\''@'\''localhost'\'' IDENTIFIED WITH mysql_native_password BY '\'''\'''
ERROR 1045 (28000): Access denied for user 'root'@'localhost' (using password: NO)

@junaruga
Copy link
Contributor Author

As a reference, when I tested the behavior on ubuntu focal container, it was succeeded by root user, though I actually used podman command rather than docker command.

$ docker run -it ubuntu:focal bash
# apt-get update -qq
# apt-get install -qq mysql-server-8.0 mysql-client-8.0
# service mysql start

# mysqld --version
/usr/sbin/mysqld  Ver 8.0.22-0ubuntu0.20.04.3 for Linux on x86_64 ((Ubuntu))

# mysql -u root -e "ALTER USER 'root'@'localhost' IDENTIFIED WITH mysql_native_password BY ''"
# mysql -u root -e 'CREATE DATABASE /*M!50701 IF NOT EXISTS */ test'

@junaruga junaruga force-pushed the wip/ci-github-actions branch from 77677ad to 4de3aad Compare January 13, 2021 16:10
@junaruga
Copy link
Contributor Author

Rebased.

On mysql 8.0 case, there is one error.
https://github.com/junaruga/mysql2/runs/1696242902?check_suite_focus=true#step:7:535

  1) Mysql2::Client session_track returns multiple session track type values when available
     Failure/Error: expect(res).to eq(["________"])

       expected: ["________"]
            got: nil

       (compared using ==)
     # ./spec/mysql2/client_spec.rb:1053:in `block (3 levels) in <top (required)>'

On mariadb 10.0 case, I see a clue here. We might do sudo mysql ... or the following SQL? I am not sure.

mysql -u root <<SQL
USE mysql;
desc mysql.user;
SELECT User, Host, Password, plugin, password FROM mysql.user;
SET PASSWORD FOR 'root'@localhost = PASSWORD("");
UPDATE mysql.user SET plugin = '' WHERE user = 'root' AND host = 'localhost';
FLUSH PRIVILEGES;
SELECT User, Host, Password, plugin, password FROM mysql.user;
SQL

@junaruga
Copy link
Contributor Author

I need someone's help to fix it.

@junaruga
Copy link
Contributor Author

Rebased. I think I am ready for the review now on this PR now.

Here is the result of GitHub Actions.
https://github.com/junaruga/mysql2/actions/runs/579780022

  • Some job are set as allow-failure: true using continue-on-error syntax. I noted the reason of the failures in .github/workflows/test.yml matrix part.
  • Added the Ubuntu cases as much as possible. Ruby 2.0.0 is not available on GitHub Actions' ruby/setup-ruby@v1 library. I found the way to run bundle install by ourselves. When we do not set bundler-cache: true on the library, we can do it. I could not add mariadb10.1, mariadb10.2, mysql55 cases that exists on Travis CI, as I have no idea how to add it.
  • The MacOSX and CentOS case with container are not included on this PR. I plan I will work on the CentOS case later for another PR.
  • There is a logic such as add-apt-repository 'http://repo.mysql.com/apt/ubuntu mysql-5.7' to set the repository from the mysql.com. The problem of add-apt-repository is even when the repository does not exist on the target ubuntu version, the command finishes successfully returning the exit status 0. This causes installing unintended version's mysql-server. To solve this problem I added wget -q --spider http://repo.mysql.com/apt/ubuntu/dists/$(lsb_release -cs)/mysql-5.7 to verify the actual repository URL.
  • I was struggled to set up the config to run mysql -u root from regular user, especially in the cases using MySQL and MariaDB installed from Ubuntu official deb package. Finally I added the full grant user root-test such as root user.

@sodabrew Could you review? If it is okay, please merge the PR and enable GitHub Actions on this repository. Thanks.

@junaruga junaruga changed the title WIP: Add GitHub Actions. Add GitHub Actions. Feb 19, 2021
@junaruga junaruga marked this pull request as ready for review February 19, 2021 00:02
@junaruga junaruga force-pushed the wip/ci-github-actions branch 2 times, most recently from 3a197d1 to 0fc0a26 Compare February 19, 2021 00:13
.travis_setup.sh Outdated
# Install the default used DB if DB is not set.
if [[ -z ${DB-} ]]; then
sudo apt-get update -qq
sudo apt-get install -qq mysql-server-5.7 mysql-client-core-5.7 mysql-client-5.7
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see this line causes an install error on the current Travis CI case on Ubuntu trusty.
https://travis-ci.org/github/brianmario/mysql2/jobs/759562170#L1200

If we still like to keep the Travis trusty case, maybe if changing mysql-server-5.7 mysql-client-core-5.7 mysql-client-5.7 to mysql-server mysql-client-core mysql-client, we might avoid the error. I wanted to add the version string of the mysql as much as possible to avoid installing the unintended version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased. The Github Actions log is here.

@junaruga junaruga force-pushed the wip/ci-github-actions branch 3 times, most recently from ae98fc1 to 00d37a7 Compare February 19, 2021 15:21
@junaruga
Copy link
Contributor Author

junaruga commented Feb 19, 2021

Rebased to pass the Travis on this PR. (Github Actions log, Travis log)

  • Add the command to create root-test DB user on CentOS case in .travis_setup_centos.sh. Without this logic, the CentOS case fails when accessing to the server with the not existing root-test DB user written in spec/configuration.yml based on spec/configuration.yml.example.
  • Remove the CI cases on Travis migrated to GitHub Actions.

@junaruga
Copy link
Contributor Author

I was struggled to set up the config to run mysql -u root from regular user, especially in the cases using MySQL and MariaDB installed from Ubuntu official deb package. Finally I added the full grant user root-test such as root user.

I tried to find a way to use the root user as a db system user instead of root-test user adding this commit on another branch in my forked repository.

The result is here. The following error happens with mariadb 10.0, as maybe the mysql_native_password is not recognized as a syntax on the old DB.

+ sudo mysql --defaults-extra-file=/etc/mysql/debian.cnf -u root -e 'ALTER USER '\''root'\''@'\''localhost'\'' IDENTIFIED WITH mysql_native_password BY '\'''\'''
ERROR 1064 (42000) at line 1: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'USER 'root'@'localhost' IDENTIFIED WITH mysql_native_password BY ''' at line 1

I think the merit of way of creating root-test user is more consistent for any testing environment than the way using the root user. Some contributors trying to run the tests also might not want to change the db system user root's settings on their local environment. In other words, this new way using root-test might confuse the contributors. Because when the granted root-test user does not exist on DB, the test failures are not obvious. Here is the result. I think the solution is to add a rake task to verify the test environment to tasks/rspec.rake before running actual tests. For example running the following command in the rake task.

$ mysql -u root-test -e 'SHOW DATABASES;'

Remove the CI cases on Travis migrated to GitHub Actions.
@junaruga junaruga force-pushed the wip/ci-github-actions branch from 00d37a7 to a9a5238 Compare February 22, 2021 11:30
@junaruga
Copy link
Contributor Author

junaruga commented Feb 22, 2021

Rebased. I was able to pass the tests without creating the root-test user. GitHub Actions log, Travis log.

@sodabrew
Copy link
Collaborator

This looks amazing. I've read it closely, I don't see anything that jumps out wrong at me. Merging now, and we can break-fix if needed!

@sodabrew sodabrew merged commit c5fa553 into brianmario:master Feb 23, 2021
@junaruga junaruga deleted the wip/ci-github-actions branch February 23, 2021 15:39
@junaruga
Copy link
Contributor Author

Thanks for your review and merging!! I am glad for that! Now we can ask people sending PRs for this repository to rebase the PR to check if the PR passes. When you see the PR passing, you see the PR to be merged.

I am working to migrate the CentOS case to GitHub Actions now.

@junaruga
Copy link
Contributor Author

we can break-fix if needed!

Yeah, it's time to do it!

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.

2 participants