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

[AN-257] Pin mysql to 8.4 for local leo and unit tests #4819

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

LizBaldo
Copy link
Collaborator

@LizBaldo LizBaldo commented Dec 19, 2024

Jira ticket: https://broadworkbench.atlassian.net/browse/AN-257

Summary of changes

What

  • Points the local leo mysql database to use 8.4 for both local runs, as well as unit testing via GHA
    Slight changes to some liquibase migration files to adapt to the mysql 8.0 -> 8.4 breaking changes around allowing FK to reference from a partial PK. Note that I added the checksum validations for each table as to not break dev and staging like [AN-256] Update mysql version to 8.0 for unit test and local leo #4802 did 🤦‍♀️

Why

  • 8.4 buys us 5 extra years compared to 8.0 so it would be great to upgrade to that!

Testing these changes

Note that we need to first merge this terra helmfile PR to update swatomation BEEs to use 8.4, then the integration tests on this PR will pass. We should then merge this PR ASAP otherwise BEE creation will fail for everybody else 😬

What to test

Who tested and where

  • This change is covered by automated tests
    • NB: Rerun automation tests on this PR by commenting jenkins retest or jenkins multi-test.
  • I validated this change
  • Primary reviewer validated this change
  • I validated this change in the dev environment

@LizBaldo LizBaldo requested a review from a team as a code owner December 19, 2024 15:58
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.77%. Comparing base (109a9a7) to head (51f0fce).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #4819   +/-   ##
========================================
  Coverage    74.77%   74.77%           
========================================
  Files          165      165           
  Lines        14954    14954           
  Branches      1187     1187           
========================================
  Hits         11182    11182           
  Misses        3772     3772           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 109a9a7...51f0fce. Read the comment docs.

Mysql 8.4+ does not allow partial keys to be referenced for a foreign key anymore, see https://bugs.mysql.com/bug.php?id=114838.
This changeSet has been modified to reflect that; the validCheckSum is the checksum from when the flag did not need to be set because the default was OFF.
</comment>
<sql> SET restrict_fk_on_non_standard_key = OFF; </sql>
Copy link
Contributor

Choose a reason for hiding this comment

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

This retroactive change would allow this Liquibase to run on a brand new MySQL 8.4 – what happens for existing CloudSQL instances like Prod?

I wonder if we also need a new changeset that will run on those and idempotently set this flag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It worked great on a preexisting BEE DB, but I can wait until I test the existing dev DB first before merging this that's a good callout

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also cool with merging as-is and revisiting in a follow-on; since this PR refers to local & tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does, but there is no rush to merge this, I can test mycloud sql locally too

Copy link
Contributor

@aednichols aednichols left a comment

Choose a reason for hiding this comment

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

Clearly an improvement; provisional thumb in case you decide to address existing CloudSQLs seprately.

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.

3 participants