Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Add no-op migrations #542

Merged
merged 9 commits into from
Mar 22, 2023
Merged

Add no-op migrations #542

merged 9 commits into from
Mar 22, 2023

Conversation

wild-endeavor
Copy link
Contributor

@wild-endeavor wild-endeavor commented Mar 17, 2023

TL;DR

The existing migrations are not correctly specified. This PR basically rewrites the existing migrations in the form that gorm recommends by adding a series of migrations that should

  1. Be a noop for existing Admin users (i.e. users who are up to date with the current set of migrations)
  2. Perfectly replicate existing Admin database in the case of a new database.

We intend to keep the current (Legacy) migrations around for one release, and then remove them, keeping only the Noop ones, which we will rename.

Type

  • Bug Fix
  • Feature
  • Plugin
  • Refactor

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

We never followed the recommended migration pattern in Gorm. To have an easier time with the work necessary to add mysql support, it'd be better to have existing model migrations cleaned up.

Testing Procedure

Sandbox

Testing using the flytectl demo sandbox was done

  • Spin up flytectl demo cluster, confirm the existing migrations were all run, then run new migrations: confirm no alter commands were run.
  • Create two new databases, run the old migrations on one, and the new migrations on the second: do a pg_dump and confirm they are identical.

The combination of these tests implies that the new migrations map one to one and onto the old migrations.

The tests were deleted but can be found here: https://gist.github.com/wild-endeavor/cf2336ebf059a087334a8d8e7efe35b1
The pg_dumps are legacy.txt and noop.txt

Tracking Issue

This is the first step in fulfilling flyteorg/flyte#3277

Signed-off-by: Yee Hing Tong <[email protected]>
@wild-endeavor wild-endeavor changed the title noop migrations Add noop migrations Mar 17, 2023
@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Merging #542 (d9ecf18) into master (dea5b2a) will decrease coverage by 0.07%.
The diff coverage is 3.74%.

❗ Current head d9ecf18 differs from pull request most recent head cc14ca6. Consider uploading reports for the commit cc14ca6 to get more accurate results

@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
- Coverage   60.18%   60.11%   -0.07%     
==========================================
  Files         169      169              
  Lines       15106    12771    -2335     
==========================================
- Hits         9091     7677    -1414     
+ Misses       5214     4293     -921     
  Partials      801      801              
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/repositories/config/migrations.go 1.01% <0.00%> (-1.97%) ⬇️
pkg/repositories/transformers/task_execution.go 75.98% <100.00%> (+3.53%) ⬆️

... and 153 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
@wild-endeavor wild-endeavor marked this pull request as ready for review March 22, 2023 00:17
Signed-off-by: Yee Hing Tong <[email protected]>
Signed-off-by: Yee Hing Tong <[email protected]>
@wild-endeavor wild-endeavor changed the title Add noop migrations Add no-op migrations Mar 22, 2023
@wild-endeavor wild-endeavor merged commit 680cac3 into master Mar 22, 2023
@wild-endeavor wild-endeavor deleted the migrations-noop branch March 22, 2023 22:23
@eapolinario eapolinario mentioned this pull request Jul 19, 2023
8 tasks
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
Signed-off-by: Yee Hing Tong <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants