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

Document how to add a database migration #23793

Closed

Conversation

delvh
Copy link
Member

@delvh delvh commented Mar 29, 2023

This was always a point of confusion for new contributors, so let's clarify it.
Especially important is the addition that updating migrations should be written to only be executed once.

@delvh delvh added topic/deployment type/docs This PR mainly updates/creates documentation outdated/backport/v1.19 This PR should be backported to Gitea 1.19 labels Mar 29, 2023
@delvh delvh added this to the 1.20.0 milestone Mar 29, 2023
@delvh delvh self-assigned this Mar 29, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 29, 2023
Comment on lines +133 to +134
In case you delete a column or table, please adapt the original migration that added it (and all subsequent migrations that use the deleted value) as well so that the column or table isn't added in the first place for new instances.
Keep the original migration as a no-op (with a comment) in case it becomes empty.
Copy link
Member

Choose a reason for hiding this comment

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

Is this what actually happens? I was always under the impression that past migrations should be left alone. There's less chance that an intermediate migration is missed that would cause some odd potential bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well… It's at least what I've observed with previous deletions, see for example #23605
we can also decide to use another process, but we should decide this now.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, tricky topic. I think leaving the original as-is may be safer than no-oping it. Treat it kind of like a git history that is never rewritten.

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory, I agree.
However, as you saw in #23605, it sometimes needs to be done.
What about a paragraph such as

If you delete a column or table, do not adapt the original migration adding it and all migrations inbetween.
There is only one exception: If the original migration was broken and didn't work on some databases, you can convert the original migration and every change in between referencing the deleted content to a no-op.

?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah sounds good. General rule should be to not alter old migrations except in the rare case they are broken, in wich case it is warranted to fix it and "rewrite history".

Copy link
Member

Choose a reason for hiding this comment

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

@delvh will you update with it?

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 1, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 8, 2023
@delvh delvh removed this from the 1.20.0 milestone Jun 7, 2023
@denyskon
Copy link
Member

denyskon commented Aug 1, 2023

@delvh It would be nice if we could have at least some docs about migrations. Are you still interested in this PR? If so, could you update your branch and resolve the open review comments?

@denyskon denyskon added issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail and removed outdated/backport/v1.19 This PR should be backported to Gitea 1.19 labels Aug 1, 2023
@denyskon denyskon removed the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Oct 3, 2023
@denyskon
Copy link
Member

denyskon commented Oct 3, 2023

Closing as no response unfortunately

@denyskon denyskon closed this Oct 3, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jan 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. topic/deployment type/docs This PR mainly updates/creates documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants