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

feat: add mariadb module #1548

Merged
merged 9 commits into from
Aug 30, 2023
Merged

Conversation

mdelapenya
Copy link
Member

@mdelapenya mdelapenya commented Aug 29, 2023

  • chore: scaffolding for mariadb module
  • feat: implement MariaDB module, using MySQL as reference
  • fix: refine mysql tests and docs

What does this PR do?

It uses the module generator to bootstrap the code for the MariaDB module, and also copies the MySQL one to implement the features we need for MariaDB.

The only differences I noticed:

  • In the my.cnf file, I had to increase the thread_stack from 128K to 256K, otherwise an error was thrown by the container startup.
  • When generating the connection string for non-TLS connections, MariaDB has security disabled by default, so the param tls=false is used in the tests.
  • I started with the mariadb:10.5.5 base image, but it does not accept the MARIADB_ prefix environment variables, as expected. That image wants MYSQL_ variables. For that reason, I moved to mariadb:11.0.3 instead, which uses the MARIADB_ prefix. Therefore, this module will copy all MARIADB_ variables to the MYSQL_ variables. @eddumelendez is this considered in the Java impl?

Why is it important?

Add a new module!

@mdelapenya mdelapenya requested a review from a team as a code owner August 29, 2023 09:26
@mdelapenya mdelapenya added the feature New functionality or new behaviors on the existing one label Aug 29, 2023
@mdelapenya mdelapenya self-assigned this Aug 29, 2023
@netlify
Copy link

netlify bot commented Aug 29, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 078dbb3
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/64eedef6fd33f100073270e0
😎 Deploy Preview https://deploy-preview-1548--testcontainers-go.netlify.app/modules/mariadb
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mdelapenya
Copy link
Member Author

Note that the sonarcloud warning is related to the existing duplication in the mariadb, which is basically shadowing what the mysql does. There subtle differences (see above), but the module is in practice the same.

@eddumelendez
Copy link
Member

  • thread_stack in tc-java is 512k. See the default config
  • About TLS, I'll test using the same image
  • From docs: From tag 10.2.38, 10.3.29, 10.4.19, 10.5.10 onwards, and all 10.6 and later tags, the MARIADB_* equivalent variables are provided. MARIADB_* variants will always be used in preference to MYSQL_* variants. So, it's still safe using MYSQL_ in tc-java

@mdelapenya
Copy link
Member Author

So, it's still safe using MYSQL_ in tc-java

I tested with 10.5.5 locally and te MARIADB_ prefix was not accepted (container failed to start because of no credentials were passed by the module container, which passed MARIADB-only variables.

Do you think we should support that version, or on the contrary keep the lower limit in the 11.0.3?

* main:
  modulegen: generate md file inside internal/mkdocs (testcontainers#1543)
  modulegen: create internal/module and internal/modfile (testcontainers#1539)
@eddumelendez
Copy link
Member

I tested with 10.5.5 locally and te MARIADB_ prefix was not accepted (container failed to start because of no credentials were passed by the module container, which passed MARIADB-only variables.

According to the docs, MARIADB_ in 10.5.x is supported from 10.5.10 onwards. So, it is expected to fail providing MARIADB_ in version 10.5.5

Do you think we should support that version, or on the contrary keep the lower limit in the 11.0.3?

11.0.3 is fairly new. In this case, I think it is ok to support previous images with MYSQL_ for more coverage. Also, as of today, there will be compatibility with new versions due to the MYSQL_ is not removed.

// It will look up for MARIADB environment variables.
func WithDefaultCredentials() testcontainers.CustomizeRequestOption {
return func(req *testcontainers.GenericContainerRequest) {
username := req.Env["MARIADB_USER"]
Copy link
Member Author

Choose a reason for hiding this comment

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

@eddumelendez would you see it convenient to check for MYSQL_ variables here? I could imagine evaluating the vars in this order:

  • check for MYSQL
  • check for MARIADB (more precedence)

Copy link
Member

Choose a reason for hiding this comment

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

interesting! However, I would rather use withUsername, withPassword and withDatabase rather than using the env vars directly. I don't think this is an issue because this is a new module, it would be if it is an existing one and just new utility fn were added but that's not the case. Those moving from GenericContainer to the module will also take that into account, I think. Historically, we haven't received issues about it in tc java. Of course, there is an assumption about using the module but if we suggest to use the module because of the pre configuration it brings then it is ok.

If we are going to plan to support MARIADB_ env vars because MYSQL_ can be deprecated then I would respect the default image's precedence.

@sonarcloud
Copy link

sonarcloud bot commented Aug 30, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
34.0% 34.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@mdelapenya mdelapenya merged commit 7c11a19 into testcontainers:main Aug 30, 2023
119 of 120 checks passed
@mdelapenya mdelapenya deleted the mariadb-module branch August 30, 2023 12:05
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Aug 30, 2023
* main: (32 commits)
  fix: remove wrong example from workspace (testcontainers#1556)
  chore(deps): bump the all group in /modules/localstack with 1 update (testcontainers#1552)
  modulegen: generate code-workspace with json marshal (testcontainers#1551)
  chore(deps): bump the all group in /modules/compose with 2 updates (testcontainers#1553)
  feat: add mariadb module (testcontainers#1548)
  feat(modulegen): print out VSCode workspace file if needed (testcontainers#1549)
  modulegen: generate md file inside internal/mkdocs (testcontainers#1543)
  modulegen: create internal/module and internal/modfile (testcontainers#1539)
  [Enhancement]: add ability to set repo:tag for ContainerRequest FromDockerfile (testcontainers#1508)
  Fix module generator for examples (testcontainers#1545)
  Update pulsar.md (testcontainers#1542)
  modulegen: create internal/make (testcontainers#1537)
  chore: fix workflow (testcontainers#1538)
  chore(deps): bump the all group in /examples/cockroachdb with 1 update (testcontainers#1522)
  chore(deps): bump the all group in /examples/bigtable with 1 update (testcontainers#1534)
  chore(deps): bump the all group in /modules/localstack with 4 updates (testcontainers#1535)
  chore(deps): bump the all group in /modules/k3s with 2 updates (testcontainers#1526)
  chore(deps): bump the all group in /examples/spanner with 2 updates (testcontainers#1532)
  chore(deps): bump the all group in /examples/firestore with 1 update (testcontainers#1523)
  chore(deps): bump the all group in /modules/redis with 1 update (testcontainers#1524)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or new behaviors on the existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants