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.

Sign up for GitHub

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

Jump to bottom

Fix migration ID #466

Closed
wants to merge 1 commit into from
Closed

Fix migration ID #466

wants to merge 1 commit into from

Conversation

gauravgahlot
Copy link
Contributor

@gauravgahlot gauravgahlot commented Mar 26, 2021 •
edited
Loading

Why is this needed

The PR changes the migration ID:
2020121691335-update-events-primary-key to 202012169135-update-events-primary-key.

@gauravgahlot gauravgahlot self-assigned this Mar 26, 2021
@gianarb
Copy link
Contributor

gianarb commented Mar 26, 2021 •
edited
Loading

we should not change old migration because it can be already been applied to some environments. Changing the ID will break the migration process for those envs. Having an ID that is not formatted as it should be is wrong but we didn't catch it on time and the cost of changing it is too high.

I am not sure what this is Get202103261030. I can't see this function anywhere in the codebase

@gauravgahlot
fix migration id
416f8da 
Signed-off-by: Gaurav Gahlot <[email protected]>
@codecov
Copy link

codecov bot commented Mar 26, 2021

Codecov Report

Merging #466 (4a53c35) into master (0f2a86f) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 4a53c35 differs from pull request most recent head 416f8da. Consider uploading reports for the commit 416f8da to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #466   +/-   ##
=======================================
  Coverage   35.82%   35.82%           
=======================================
  Files          47       47           
  Lines        2892     2892           
=======================================
  Hits         1036     1036           
  Misses       1763     1763           
  Partials       93       93
Impacted Files Coverage Δ
db/migration/migration.go 100.00% <ø> (ø)
...igration/202012169135-update-events-primary-key.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

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

@gauravgahlot
Copy link
Contributor Author

we should not change old migration because it can be already been applied to some environments. Changing the ID will break the migration process for those envs. Having an ID that is not formatted as it should be is wrong but we didn't catch it on time and the cost of changing it is too high.

I am not sure what this is Get202103261030. I can't see this function anywhere in the codebase

Get202103261030 was pushed by mistake.

Due to incorrect ID, this migration is not applied in the correct order. This results in errors when remove events table in a separate migration.

@gauravgahlot gauravgahlot requested review from gianarb and removed request for Cbkhare and parauliya March 26, 2021 08:02
@gianarb
Copy link
Contributor

gianarb commented Mar 26, 2021

Are you sure?! Technically the migrations are applied in the order they are appended to the slice! let's zoom so we can double check!

gianarb pushed a commit that referenced this pull request Mar 26, 2021
Add a test to assert that migrations are ordered as we declared them
4416404 
This tests is coming from this PR
#466

We didn't fully understood how the migration library sorts migration
before having to troubleshoot that PR. Even if we declare them in order
as part of the GetMigrations function the library still applies quick
sort on the migration ID to stay concurrency safe

https://github.com/rubenv/sql-migrate/blob/011dc47c6043b25483490739b61cabbc5da7ee9a/migrate.goL216

In order to improve the visbility and to akwnoldge that the way we
append migrations to GetMigrations is the same we get after the sorting
we wrote this tests.
@gianarb gianarb mentioned this pull request Mar 26, 2021
Merged
gianarb pushed a commit that referenced this pull request Mar 26, 2021
Add a test to assert that migrations are ordered as we declared them
2bb0a9f 
This tests is coming from this PR
#466

We didn't fully understood how the migration library sorts migration
before having to troubleshoot that PR. Even if we declare them in order
as part of the GetMigrations function the library still applies quick
sort on the migration ID to stay concurrency safe

https://github.com/rubenv/sql-migrate/blob/011dc47c6043b25483490739b61cabbc5da7ee9a/migrate.goL216

In order to improve the visbility and to akwnoldge that the way we
append migrations to GetMigrations is the same we get after the sorting
we wrote this tests.

Signed-off-by: Gianluca Arbezzano <[email protected]>
gianarb pushed a commit that referenced this pull request Mar 26, 2021
Add a test to assert that migrations are ordered as we declared them
8ec9ce1 
This tests is coming from this PR
#466

We didn't fully understood how the migration library sorts migration
before having to troubleshoot that PR. Even if we declare them in order
as part of the GetMigrations function the library still applies quick
sort on the migration ID to stay concurrency safe

https://github.com/rubenv/sql-migrate/blob/011dc47c6043b25483490739b61cabbc5da7ee9a/migrate.goL216

In order to improve the visbility and to akwnoldge that the way we
append migrations to GetMigrations is the same we get after the sorting
we wrote this tests.

Signed-off-by: Gianluca Arbezzano <[email protected]>
gianarb pushed a commit that referenced this pull request Mar 26, 2021
Add a test to assert that migrations are ordered as we declared them
8face1a 
This tests is coming from this PR
#466

We didn't fully understood how the migration library sorts migration
before having to troubleshoot that PR. Even if we declare them in order
as part of the GetMigrations function the library still applies quick
sort on the migration ID to stay concurrency safe

https://github.com/rubenv/sql-migrate/blob/011dc47c6043b25483490739b61cabbc5da7ee9a/migrate.goL216

In order to improve the visbility and to akwnoldge that the way we
append migrations to GetMigrations is the same we get after the sorting
we wrote this tests.

Signed-off-by: Gianluca Arbezzano <[email protected]>
mergify bot added a commit that referenced this pull request Mar 26, 2021
@mergify
Add a test to assert that migrations are ordered as we declared them (#…
a49e7e6 
…467)

This tests is coming from this PR
#466

We didn't fully understood how the migration library sorts migration
before having to troubleshoot that PR. Even if we declare them in order
as part of the GetMigrations function the library still applies quick
sort on the migration ID to stay concurrency safe

https://github.com/rubenv/sql-migrate/blob/011dc47c6043b25483490739b61cabbc5da7ee9a/migrate.goL216

In order to improve the visbility and to akwnoldge that the way we
append migrations to GetMigrations is the same we get after the sorting
we wrote this tests.

I will start a conversation with the lib maintainer to figure out how we can improve our current workflow.
@gianarb
Copy link
Contributor

gianarb commented Mar 26, 2021

This is not solved by #467

@gianarb gianarb closed this Mar 26, 2021
@gauravgahlot gauravgahlot deleted the fix-migration-id branch March 26, 2021 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gianarb gianarb Awaiting requested review from gianarb

Assignees

@gauravgahlot gauravgahlot

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gauravgahlot @gianarb