Skip to content

Conversation

Al2Klimov
Copy link
Member

just to be sure we're compatible with this RDBMS and not only MySQL.

- name: mysql
pretty_name: MySQL
image: mysql:latest
- name: mysql
Copy link
Member

Choose a reason for hiding this comment

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

Having another test named mysql actually being MariaDB is a bit confusing. Why not setting the name to what it is and introduce another field matching ICINGADB_TESTS_DATABASE_TYPE, e.g., database_type?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please have a look at https://github.com/Icinga/icingadb/blob/main/.github/workflows/sql.yml. This also avoids setting variables like ICINGA_TESTING_PGSQL_IMAGE: mysql:latest.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, a such env var is a bit cheaty, but I prefer a nice looking matrix with the DB type written just one (lowercase).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, a such env var is a bit cheaty,

And setting ICINGA_TESTING_PGSQL_IMAGE: mysql:latest is not cheaty at all?

but I prefer a nice looking matrix with the DB type written just one

You mean once? https://github.com/Icinga/icingadb/blob/main/.github/workflows/sql.yml doesn't repeat it any more than the current version of this PR.

(lowercase).

That sounds just like an arbitrary choice. Sure, if both were otherwise identical, I'd also pick lowercase, but they aren't: it's easier to make the string lowercase in our Go code than in GitHub Actions Workflow YAML. And I don't think "I prefer lowercase" is a reasonable argument to completely rule out the alternative.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, setting such an env var IS cheaty of course (as I said), but I prefer it over

  • having to lower/upper-case something somewhere
  • having to repeat the database type more than once in the matrix

Copy link
Member

Choose a reason for hiding this comment

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

Please have a look at https://github.com/Icinga/icingadb/blob/main/.github/workflows/sql.yml. This also avoids setting variables like ICINGA_TESTING_PGSQL_IMAGE: mysql:latest.

I don't understand why it is not done exactly as in the referenced file.

@Al2Klimov Al2Klimov force-pushed the Al2Klimov-patch-2 branch from 9a88abc to 55f0e41 Compare April 5, 2024 09:50
@cla-bot cla-bot bot added the cla/signed label Apr 5, 2024
uses: actions/upload-artifact@v2
with:
name: ${{ matrix.database.name }}-debug.log.xz
name: ${{ matrix.database.type }}-debug.log.xz
Copy link
Member

Choose a reason for hiding this comment

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

With this change, the debug log from MariaDB and MySQL are having the same filename now. I would advise to revert this.

@oxzi
Copy link
Member

oxzi commented Jun 17, 2025

While this PR was somewhat forgotten, I just thought about it in spite of #947. How about testing both the latest version as well as the minimum required version for each DBMS?

@Al2Klimov
Copy link
Member Author

They're either not available or don't work and it's a bit out of scope here.

@Al2Klimov Al2Klimov requested a review from oxzi August 20, 2025 11:44
@julianbrost
Copy link
Contributor

They're either not available or don't work and it's a bit out of scope here.

I wouldn't understand that comment if I didn't happen to look at this PR and it's pipelines at just the right moment by pure chance, i.e. at this run: https://github.com/Icinga/icingadb/actions/runs/17097225377

One job failed because there seems to be no mariadb:10.2.2 image on Docker Hub:

panic: Error response from daemon: manifest for mariadb:10.2.2 not found: manifest unknown: manifest unknown

The other one failed with a MySQL error:

panic: Error 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'ON . TO u1 */' at line 1

This also needs more context: that's an error from the database setup done by icinga-testing.

@julianbrost
Copy link
Contributor

Fixing that is trivial actually (if you know where to look), so I just created a PR for it: Icinga/icinga-testing#32

@Al2Klimov Al2Klimov force-pushed the Al2Klimov-patch-2 branch 2 times, most recently from 46be2b2 to 39f2210 Compare September 15, 2025 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants