Skip to content

CI: Simplify Changes to Explicitly Install and Configure MySQL8#9370

Merged
deepthi merged 2 commits intovitessio:mainfrom
planetscale:cimysqlfix
Dec 14, 2021
Merged

CI: Simplify Changes to Explicitly Install and Configure MySQL8#9370
deepthi merged 2 commits intovitessio:mainfrom
planetscale:cimysqlfix

Conversation

@mattlord
Copy link
Copy Markdown
Member

@mattlord mattlord commented Dec 14, 2021

Description

Limiting the new/additional work we do in the CI in order to limit the time and effort required to meet the primary objective: ensure that we deterministically install the latest MySQL 8.0 release from the official MySQL APT repo.

Related Issue(s)

Follow-up on: #9368
Caused by: actions/runner-images#4674

Checklist

  • Should this PR be backported? 12.0 maybe?
  • Tests were added or are not required
  • Documentation is not required

Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord mattlord changed the title testing... CI: Simplify Changes to Explicitly Install and Configure MySQL8 Dec 14, 2021
@mattlord mattlord marked this pull request as ready for review December 14, 2021 01:42
@mattlord mattlord requested a review from deepthi December 14, 2021 01:42

- name: Get dependencies
run: |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@GuptaManan100 pointed out that this file is actually generated (see Line 1) using https://github.com/vitessio/vitess/blob/main/test/ci_workflow_gen.go.
I'm not sure what happens to other tests if we change the template that is being used by the make target to generate yaml files.
I guess you will need to try and move these changes into the template and if it breaks other tests, then maybe create a new template for mysql80?

Copy link
Copy Markdown
Member Author

@mattlord mattlord Dec 14, 2021

Choose a reason for hiding this comment

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

Yeah, will need to do something there. Running make generate_ci_workflow puts cluster_endtoend_mysql80.yml back into its original state where it breaks after the ubuntu-latest image change.

Copy link
Copy Markdown
Member Author

@mattlord mattlord Dec 14, 2021

Choose a reason for hiding this comment

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

Added support here: 9f6bd06

So now make generage_ci_workflow produces what I had previously done in the generated file.

Signed-off-by: Matt Lord <mattalord@gmail.com>
Copy link
Copy Markdown
Contributor

@GuptaManan100 GuptaManan100 left a comment

Choose a reason for hiding this comment

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

LGTM!
We can even remove the ubuntu version check. Like make all the tests run on 18.04. As far as I know, we were only using 20.04 to get the mysql 8.0 pre-installed


type clusterTest struct {
Name, Shard string
Name, Shard, Platform string
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is elegant.
Only one nit, Platform doesn't seem like the right term for database (hat is what we are changing here).
How about using Flavor since we use that elsewhere in the code base?

Copy link
Copy Markdown
Member Author

@mattlord mattlord Dec 14, 2021

Choose a reason for hiding this comment

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

I had similar thoughts, but:

  1. We already use Platform this way (e.g. mysql57, mariadb102) for the unit tests and the self hosted cluster e2e tests in the same file
  2. We already use OS, and Flavor is used for the GTID type so the Flavor is mysql56 for 5.7 and 8.0 in those contexts

I'm fine changing it to whatever though.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

alright, you've convinced me :)
If we are already using Platform, it makes sense to keep it consistent.

@deepthi deepthi merged commit 6646571 into vitessio:main Dec 14, 2021
@deepthi deepthi deleted the cimysqlfix branch December 14, 2021 22:25
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.

3 participants