-
Notifications
You must be signed in to change notification settings - Fork 105
Add new subcommand to update migrations that contain breaking changes #855
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
Conversation
8ceb579
to
dbe2d33
Compare
20340d9
to
97926a0
Compare
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.
Looks good; left some comments 👍
cmd/update.go
Outdated
} | ||
|
||
updateCmd.Flags().BoolVarP(&useJSON, "json", "j", false, "Output migration file in JSON format instead of YAML") | ||
updateCmd.Flags().BoolVarP(&local, "local", "l", false, "Update all local migration files") |
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.
I don't think we need this flag. IMO the update
command should awlays operate on all migrations in the given directory.
I'm not sure what the use-case is for only updating the local migations that have yet to be applied.
In any case, source control allows users to only commit the updates they want.
I'd remove the flag and make the command always operate on all migrations.
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 --local
flag is running on all local files, not just unapplied ones. By default pgroll updates only unapplied migrations. I think the flag is valuable when you pull all migrations from the database and run the command with --local
. Or when you do not specify the flag you just update the local, unapplied files because we do not want to rewrite the history in the database. WDYT?
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.
I'm still in favour of removing the flag and having the command always run on all files in the local directory.
The use case for this command AIUI is:
- A user upgrades
pgroll
from versionx
to versiony
. - Version
y
makes a breaking change to the YAML/JSON format for a particular operation. - The user has already applied some migrations in the old format to their target database.
Now as a user I want to get all migrations into the new format so that I have a set of migration files for my project that I can use with the version y
of pgroll
.
- The user runs
pgroll update migrations/
to rewrite the migration files. - The user commits those changes to source control.
Now I can bootstrap other DBs from scratch with the migration files in migrations/
.
I don't see the use case for only updating the migration files that have yet to be applied to the database?
we do not want to rewrite the history in the database
This command only ever touches local files, not migrations in the database though right?
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.
This command only ever touches local files, not migrations in the database though right?
Yes.
f3a5cb1
to
4c686c1
Compare
This command updates migrations that contain breaking changes.
4c686c1
to
8466301
Compare
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.
Looks good; left a few comments.
cmd/update.go
Outdated
} | ||
|
||
updateCmd.Flags().BoolVarP(&useJSON, "json", "j", false, "Output migration file in JSON format instead of YAML") | ||
updateCmd.Flags().BoolVarP(&local, "local", "l", false, "Update all local migration files") |
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.
I'm still in favour of removing the flag and having the command always run on all files in the local directory.
The use case for this command AIUI is:
- A user upgrades
pgroll
from versionx
to versiony
. - Version
y
makes a breaking change to the YAML/JSON format for a particular operation. - The user has already applied some migrations in the old format to their target database.
Now as a user I want to get all migrations into the new format so that I have a set of migration files for my project that I can use with the version y
of pgroll
.
- The user runs
pgroll update migrations/
to rewrite the migration files. - The user commits those changes to source control.
Now I can bootstrap other DBs from scratch with the migration files in migrations/
.
I don't see the use case for only updating the migration files that have yet to be applied to the database?
we do not want to rewrite the history in the database
This command only ever touches local files, not migrations in the database though right?
Co-authored-by: Andrew Farries <[email protected]>
Co-authored-by: Andrew Farries <[email protected]>
Co-authored-by: Andrew Farries <[email protected]>
Updated the PR. I will update the guide in my other PR after this gets merged. |
Co-authored-by: Andrew Farries <[email protected]>
This PR adds more details to `update` command. Requires #855 --------- Co-authored-by: Andrew Farries <[email protected]>
This PR adds a new subcommand called
update
. It updates migrations that contain breaking changes in the selected folder.Dev notes
The new
migrations.FileUpdater
follows the chain of responsibility design pattern. In theFileUpdater
you can register new updater functions (migrations.updaterFn
) for any pgroll migration. The file updater iterates over all operations in a migration and run the registered updater functions in order.The ordering of the updater functions is important. Please always add a new updater function to the end of the list.
Closes #770