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.

Sign up for GitHub

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

Jump to bottom

Add Index to action.user_id #27403

Merged
merged 3 commits into from
Oct 4, 2023
Merged

Add Index to action.user_id #27403

merged 3 commits into from
Oct 4, 2023

Conversation

JakobDev
Copy link
Contributor

@JakobDev JakobDev commented Oct 2, 2023

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 2, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 2, 2023
@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 Oct 2, 2023
techknowlogick
techknowlogick previously requested changes Oct 2, 2023
View reviewed changes
models/migrations/v1_21/v279.go
// Copyright 2023 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT

package v1_21 //nolint
Copy link
Member

Choose a reason for hiding this comment

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

Now we are in 1.22 dev cycle could you move this to the new package?

Copy link
Member

Choose a reason for hiding this comment

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

Would that prevent a backport?

Copy link
Member

Choose a reason for hiding this comment

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

I also think it would be better to backport it to 1.21 instead of treating it as a 1.22 feature.

Copy link
Member

Choose a reason for hiding this comment

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

Since we have created v1.21 branch, we should not do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is made to be backported to 1.21, as it improves the performance and does not introduce new features

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have any 1.22 migration yet, so I don't see the problem.

Copy link
Member

Choose a reason for hiding this comment

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

@techknowlogick @lunny any objections/other ideas?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @silverwind that we need to update how we do migrations, preferably tracking all migrations that are done instead of a counter (I'm a fan of how rails does their migrations). I wrote a library for exactly this for xorm, https://github.com/techknowlogick/xormigrate which is used by a few projects, and does allow for splitting things out into directories to ensure 100s of files aren't all in the same directory. This however is a larger conversation than just this PR.

Comment scoped to this PR: My personal preference would be to change it to 1.22 as that's the dev cycle we are in, and then backport it with the same 1.22 directory. However we are in the stage where migrations can still be backported, and 1.21 is not fully released yet, so I can see using 1.21 still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the directory should not represent the development circle. It should represent for which version the migration is.

Copy link
Member

@silverwind silverwind Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ultimately it won't matter if the 1.21 has files in the 1.22 folder. so let's go with that.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 2, 2023
@delvh delvh added the backport/v1.21 This PR should be backported to Gitea 1.21 label Oct 3, 2023
@delvh delvh added the performance/speed performance issues with slow downs label Oct 3, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Oct 3, 2023
@denyskon denyskon added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 3, 2023
@silverwind silverwind enabled auto-merge (squash) October 3, 2023 20:23
@silverwind silverwind merged commit 4636f56 into go-gitea:main Oct 4, 2023
24 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Oct 4, 2023
GiteaBot added a commit to GiteaBot/gitea that referenced this pull request Oct 4, 2023
@JakobDev @GiteaBot
Add Index to action.user_id (go-gitea#27403)
c81b9fe 
Another Column that needs a Index. Found at
https://codeberg.org/forgejo/discussions/issues/61#issuecomment-1258744.

Co-authored-by: Giteabot <teabot@gitea.io>
@GiteaBot GiteaBot mentioned this pull request Oct 4, 2023
Merged
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Oct 4, 2023
@JakobDev JakobDev deleted the actionindex branch October 4, 2023 05:56
silverwind pushed a commit that referenced this pull request Oct 4, 2023
@GiteaBot @JakobDev
Add Index to action.user_id (#27403) (#27425)
290440e 
Backport #27403 by @JakobDev

Another Column that needs a Index. Found at
https://codeberg.org/forgejo/discussions/issues/61#issuecomment-1258744.

Co-authored-by: JakobDev <jakobdev@gmx.de>
silverwind added a commit to silverwind/gitea that referenced this pull request Oct 4, 2023
@silverwind
Merge main, rename to gitea-auto
454e475 
* origin/main:
  When comparing with an non-exist repository, return 404 but 500 (go-gitea#27437)
  Fix pr template (go-gitea#27436)
  Use minimal required version on CI and remove unnecessary services (go-gitea#27429)
  Fix  missing `ctx`  in new_form.tmpl  (go-gitea#27434)
  Use flex-container for repo and org settings (go-gitea#27418)
  Fix yet another `ctx` template bug (go-gitea#27417)
  Add Index to `action.user_id` (go-gitea#27403)
  [skip ci] Updated translations via Crowdin
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jan 2, 2024
@lunny
Copy link
Member

lunny commented Mar 30, 2024 •
edited by techknowlogick
Loading

This will be ignored because indexes have been defined in line 63 TableIndices. 4636f56#r140396917

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@delvh delvh delvh approved these changes

@techknowlogick techknowlogick techknowlogick left review comments

@silverwind silverwind silverwind approved these changes

@lunny lunny lunny left review comments

@denyskon denyskon denyskon approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. performance/speed performance issues with slow downs size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.
7 participants
@JakobDev @lunny @silverwind @techknowlogick @denyskon @delvh @GiteaBot