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 MariaDB to CI configuration #307

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tabarnhack
Copy link

According to #257, MariaDB has been added to CI configuration.
The minimal version taken of MariaDB is 10.2 as the 10.1 has an EOL on October 2020, thus this version is obsolete.

@AlekSi
Copy link
Member

AlekSi commented Oct 23, 2021

@tabarnhack But those changes are not enough to actually test agains MariaDB – you have to change Makefile too

There was no test step about Mariadb in the Makefile. 2 have been added
to mimic the tests performed on MySQL.
Moreover, the docker-compose file has been altered in order to add the
MariaDB image. As MariaDB and MySQL are similar, the listening port for
the first one has been moved from 3306 to 6033 to prevent any conflict.
The connection strings have been modified to take the change into
account.
@tabarnhack
Copy link
Author

Indeed, I did not read the codebase enough to understand where the tests was made.
I've made the changes. It should be good now.

@codecov
Copy link

codecov bot commented Oct 24, 2021

Codecov Report

Merging #307 (752c63f) into main (ba989a9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #307   +/-   ##
=======================================
  Coverage   68.57%   68.57%           
=======================================
  Files          20       20           
  Lines        1715     1715           
=======================================
  Hits         1176     1176           
  Misses        484      484           
  Partials       55       55           

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 ba989a9...752c63f. Read the comment docs.

Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated
mariadb-traditional: export REFORM_TEST_ADMIN_SOURCE = root@tcp(localhost:6033)/mysql
mariadb-traditional: export REFORM_TEST_INIT_SOURCE = root@tcp(localhost:6033)/reform-database?parseTime=true&clientFoundRows=true&time_zone='UTC'&sql_mode='ANSI'&multiStatements=true
mariadb-traditional: export REFORM_TEST_SOURCE = root@tcp(localhost:6033)/reform-database?parseTime=true&clientFoundRows=true&time_zone='America%2FNew_York'&sql_mode='TRADITIONAL'&interpolateParams=true
mariadb-traditional: export REFORM_TEST_COVER=mysql
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mariadb-traditional: export REFORM_TEST_COVER=mysql
mariadb-traditional: export REFORM_TEST_COVER=mariadb

The same for mariadb-traditional

Copy link
Author

Choose a reason for hiding this comment

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

I didn't change as the syntax between the 2 projects has a great compatibility and SQL scripts should run seemlessly on MySQL as well as MariaDB (source: MariaDB vs MySQL). Otherwise, it would have been duplicating the mysql scripts and renaming as mariadb.

This duplicate produced a warning while running make
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