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

Upgrade xorm to v1.3.9 and improve some migrations Sync #29899

Merged
merged 3 commits into from
Jul 14, 2024

Conversation

lunny
Copy link
Member

@lunny lunny commented Mar 19, 2024

No description provided.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 19, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 19, 2024
Comment on lines +16 to +22
if _, err := x.SyncWithOptions(xorm.SyncOptions{
IgnoreIndices: true,
IgnoreConstrains: true,
}, new(HookTask)); err != nil {
return err
}
_, err := x.Exec("UPDATE hook_task SET payload_version = 1 WHERE payload_version IS NULL")
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to switch these statements around.
Otherwise, it seems inconsistent.
Why is it even necessary, even though it wasn't previously?

Copy link
Member Author

Choose a reason for hiding this comment

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

This maybe an unrelated change to this PR. I just found v290 missed the update operations. Because creating a new column with default value will not update the old record automatically. It should be a bug of previous PR.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe split this change out into separate PR?

Copy link
Member

Choose a reason for hiding this comment

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

I think fixing it here is fine ... just add a doctor command to address this things ?

@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 Mar 25, 2024
@KN4CK3R
Copy link
Member

KN4CK3R commented Apr 1, 2024

How to decide when to ignore indices and constrains?

@lunny
Copy link
Member Author

lunny commented Apr 1, 2024

How to decide when to ignore indices and constrains?

It depends on the context of the migration. If the migration just want to create a new index, then it can ignore deleting index.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 12, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 14, 2024
@techknowlogick techknowlogick merged commit 0d08bb6 into go-gitea:main Jul 14, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Jul 14, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 14, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 16, 2024
* giteaofficial/main:
  Code editor theme enhancements (go-gitea#31629)
  Add option to change mail from user display name (go-gitea#31528)
  Upgrade xorm to v1.3.9 and improve some migrations Sync (go-gitea#29899)
  Issue Templates: add option to have dropdown printed list (go-gitea#31577)
  Fix update flake (go-gitea#31626)
silverwind added a commit to techknowlogick/gitea that referenced this pull request Jul 23, 2024
* origin/main: (59 commits)
  fix OIDC introspection authentication (go-gitea#31632)
  Enable direnv (go-gitea#31672)
  [skip ci] Updated translations via Crowdin
  [skip ci] Updated translations via Crowdin
  fix redis dep (go-gitea#31662)
  add skip secondary authorization option for public oauth2 clients (go-gitea#31454)
  Fix a branch divergence cache bug (go-gitea#31659)
  [skip ci] Updated translations via Crowdin
  Remove unneccessary uses of `word-break: break-all` (go-gitea#31637)
  [skip ci] Updated translations via Crowdin
  Allow searching issues by ID (go-gitea#31479)
  allow synchronizing user status from OAuth2 login providers (go-gitea#31572)
  Enable `no-jquery/no-class-state` (go-gitea#31639)
  Added default sorting milestones by name (go-gitea#27084)
  Code editor theme enhancements (go-gitea#31629)
  Add option to change mail from user display name (go-gitea#31528)
  Upgrade xorm to v1.3.9 and improve some migrations Sync (go-gitea#29899)
  Issue Templates: add option to have dropdown printed list (go-gitea#31577)
  Fix update flake (go-gitea#31626)
  [skip ci] Updated translations via Crowdin
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/dependencies modifies/go Pull requests that update Go code modifies/migrations size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants