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

chore: fix migration conflicts #9097

Closed
wants to merge 15 commits into from
Closed

Conversation

mattkrick
Copy link
Member

@mattkrick mattkrick commented Nov 1, 2023

Description

The most frequent reason for a failed release is a failed predeploy due to conflicting migration names. For example, an older PR gets merged after a newer PR was already merged & pushed to prod, invalidating the migration run order.

This PR stops that by making sure the PR doesn't include any migrations that predate the latest migration on master.

DEMO

If you have no migrations in your PR, you get this
Screenshot 2023-11-01 at 5 21 35 PM

If you have 1 valid migration in your PR, you get this
Screenshot 2023-11-01 at 5 21 19 PM

If you have 1 migration and it predates the latest migration on master, you get this
Screenshot 2023-11-01 at 5 20 14 PM

If you have 2 migrations in your PR you should consolidate or break apart your PR, so you get this
Screenshot 2023-11-01 at 5 20 06 PM

Signed-off-by: Matt Krick <matt.krick@gmail.com>
@github-actions github-actions bot added the size/s label Nov 1, 2023
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
@mattkrick mattkrick changed the title chore: fix migraiton conflicts, untested chore: fix migration conflicts Nov 2, 2023
Copy link
Contributor

@Dschoordsch Dschoordsch left a comment

Choose a reason for hiding this comment

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

This will only catch some of the cases. If I have 2 PRs with conflicting migrations, both will pass the build pre-merge, but might cause an issue post merge as they're not re-verified after each master change.
I think we could only reliably check this on master post-merge.

Comment on lines +56 to +60
elif [[ 'OLD_MIGRATIONS_COUNT + 1' -lt 'NEW_MIGRATIONS_COUNT' ]]; then
# Necessary since we only check the newest migration, not each one
echo Please only PR 1 Migration at a time
exit 1
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 I'm not a fan of this restriction. If you know the number of migrations, you can do

FIRST_NEW_MIGRATION=$(ls packages/server/postgres/migrations | tail -n $NEW_MIGRATION_COUNT | head -n1)

Copy link
Contributor

@Dschoordsch Dschoordsch left a comment

Choose a reason for hiding this comment

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

After thinking about it, I think I would prefer if we could just check master that it's in the correct order. I think something along the lines of this

#!/bin/bash

PREV_COMMIT=$(git rev-list HEAD | tail -n 1)
PREV_FILE=''
for F in packages/server/postgres/migrations/*.ts; do
  CURR_COMMIT=$( git log --diff-filter=A --pretty=format:"%H" -- "$F")
  git merge-base --is-ancestor $PREV_COMMIT $CURR_COMMIT > /dev/null
  if [ $? -ne 0 ]; then
    echo "$PREV_FILE was added after $F but has a smaller timestamp"
  fi
  PREV_COMMIT=$CURR_COMMIT
  PREV_FILE=$F
done

This needs some cleanup though, or we make sure to only consider all files after the last failure

@mattkrick
Copy link
Member Author

mattkrick commented Nov 2, 2023

We should probably break this out into its own action. That way it could run when the PR gets opened & re-run whenever a commit to master gets pushed. I'll toy around a bit more!

@mattkrick
Copy link
Member Author

mattkrick commented Nov 3, 2023

omg i don't think it's possible to trigger a workflow when the base branch gets updated https://github.com/orgs/community/discussions/64119

i found a solution.
if we require that PRs be up to date before merging, then before a PR gets merged, they'll have to merge master into it & that will trigger a new action... this is safe, but it also really sucks because that means we can't merge 2 PRs back to back.
https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#require-status-checks-before-merging

@mattkrick mattkrick closed this Nov 10, 2023
@mattkrick mattkrick deleted the chore/no-migration-conflicts branch January 23, 2024 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants