-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
feat(migrations) add --force flag #4071
Conversation
Improves reentrancy of migrations, by making sure each operation has its own exception. In practice this means avoiding constructs such as: ``` DO $$ BEGIN ALTER TABLE IF EXISTS ONLY "routes" ADD "name" TEXT UNIQUE; ALTER TABLE IF EXISTS ONLY "routes" ADD "snis" TEXT UNIQUE; EXCEPTION WHEN DUPLICATE_COLUMN THEN -- Do nothing, accept existing state END; $$; ``` in which "name" was declared in one release candidate and "snis" in the next. By sharing the same exception, running this migration on the latter rc would throw on the first `ALTER TABLE` and never run the second one. It also ensures that `run_on` is added to the Cassandra migration during the `up` phase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall; only slight concerns with regards to end-use experience.
no_wait = true, -- exit the mutex if another node acquired it | ||
} | ||
if opts.force then | ||
log("cannot use --force with 'finish'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's keep the language consistent: cannot vs can't (above). Which should pick one and stick with it (any of the two is fine imho).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That module already had 2 can't
s and 2 cannot
s in other messages :) Will standardize them on cannot
for 🤖 computer voice 🤖 reasons, then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
|
||
assert(db:run_migrations(schema_state.executed_migrations, { | ||
run_up = true, | ||
})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error messages following this branch would look slightly weird imho. E.g.:
forcing re-execution of these migrations: [...]
...
...
database is already up-to-date
And:
forcing re-execution of these migrations: [...]
...
...
migrations to run: [...]
It would be enough to slightly update these logs so that their text makes sense in both contexts (forced and not forced migrations). Or to wrap them in appropriate branches determining if the --force
flag is in use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regarding the messages, I think just removing the database is already up-to-date
message in the forced case would be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, sounds good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
Adds a `--force` flag which allows for re-running the `up` migrations in between release candidate releases. This is a stopgap measure due to the absence of separate migration files in the previous release candidates of Kong 1.0, which should allow databases built with previous release candidates to be upgraded to the latest one.
5be8c73
to
ab441c3
Compare
Adds a
--force
flag tokong migrations up
to allow for migrating from one release candidate to the next (since the name of the migration file is unchanged).A more robust solution should be in place for future rc cycles, but this might be sufficient for now. Tested locally by:
next
kong migrations up --force
Verified to fail with the errors seen by users when skipping step 4 and to work with Postgres and Cassandra when including it.
See #4033, #4042, #4049.