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

Optimize branch protection rule loading #32280

Merged
merged 18 commits into from
Oct 29, 2024
Merged

Optimize branch protection rule loading #32280

merged 18 commits into from
Oct 29, 2024

Conversation

6543
Copy link
Member

@6543 6543 commented Oct 16, 2024 •
edited
Loading

before if it was nonglob each load would try to glob it and the check that is not glob ... now we only do that once and no future loading will trigger it

Sponsored by Kithara Software GmbH

@6543 6543 added the type/enhancement An improvement of existing functionality label Oct 16, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 16, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 16, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Oct 16, 2024
@6543
Copy link
Member Author

6543 commented Oct 16, 2024

the followup work will make glob rules order by user define priority ... so you dont have to delete and recreate rules just to have the correct order ...

@confusedsushi
Copy link
Contributor

I suggest to not handle the non-glob rules differently. I can imagine a order of rules where a non-glob rule might should kick in between two glob rules. Also from a user perspective it is probably confusing why the one rule is added "in the middle" while the other is added to the end.

If you handle both type the same the user interface becomes easier understandable.

@6543 6543 changed the title Order branch protection rule that are not globs by name Optimize branch protection rule loading Oct 17, 2024
@6543 6543 added type/refactoring Existing code has been cleaned up. There should be no new functionality. and removed type/enhancement An improvement of existing functionality labels Oct 17, 2024
@6543 6543 mentioned this pull request Oct 17, 2024
Draft
6 tasks
@6543 6543 requested a review from lunny October 17, 2024 09:52
@lunny
Copy link
Member

lunny commented Oct 20, 2024

I would like to priority rather than this one.

@6543
Copy link
Member Author

6543 commented Oct 21, 2024

@lunny all changes related sorting are moved out and will be done in #32286

this is now just a pull to refactor and add more test coverage

@6543
Copy link
Member Author

6543 commented Oct 21, 2024

before if it was nonglob each load would try to glob it and the check that is not glob ... now we only do that once and no future loading will trigger it

@6543
Copy link
Member Author

6543 commented Oct 21, 2024

@lunny updated pull description ... I missed that one sorry

lunny
lunny reviewed Oct 22, 2024
View reviewed changes
models/git/protected_branch.go
protectBranch.globRule, err = glob.Compile(protectBranch.RuleName, '/')
if err != nil {
log.Warn("Invalid glob rule for ProtectedBranch[%d]: %s %v", protectBranch.ID, protectBranch.RuleName, err)
protectBranch.globRule = glob.MustCompile(glob.QuoteMeta(protectBranch.RuleName), '/')
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the meaning of this line.

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 I also dont know the reasoning of this, but since I dont know why it was initially added i should not remove it randomly on refactoring ...

Copy link
Contributor

Choose a reason for hiding this comment

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

@lunny but the code was written by you : Supports wildcard protected branch #20825

Copy link
Member

Choose a reason for hiding this comment

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

I need to add some comments there.

Copy link
Member Author

Choose a reason for hiding this comment

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

feel free to push to my branch

Copy link
Member Author

@6543 6543 Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ok looks like this should be done in another dedicated pull - the topic is holding this minor refactoring back

(and is off topic of this pull)

@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 22, 2024
@6543 6543 requested review from a team and wxiaoguang October 28, 2024 17:03
@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 Oct 29, 2024
@6543 6543 merged commit 5d43801 into go-gitea:main Oct 29, 2024
26 checks passed
@6543 6543 deleted the order-BranchProtection-non-glob-by-name branch October 29, 2024 14:43
@lunny lunny added this to the 1.23.0 milestone Oct 29, 2024
silverwind added a commit to silverwind/gitea that referenced this pull request Oct 30, 2024
@silverwind
Merge remote-tracking branch 'origin/main' into types-4
5600f34 
* origin/main: (21 commits)
  Fix toAbsoluteLocaleDate and add more tests (go-gitea#32387)
  Respect UI.ExploreDefaultSort setting again (go-gitea#32357)
  Fix absolute-date (go-gitea#32375)
  Fix undefined errors on Activity page (go-gitea#32378)
  Add new [lfs_client].BATCH_SIZE and [server].LFS_MAX_BATCH_SIZE config settings. (go-gitea#32307)
  remove unused call to $.HeadRepo in view_title template (go-gitea#32317)
  Fix clean tmp dir (go-gitea#32360)
  Optimize branch protection rule loading (go-gitea#32280)
  Suggestions for issues (go-gitea#32327)
  Migrate vue components to setup (go-gitea#32329)
  Fix db engine (go-gitea#32351)
  Refactor the DB migration system slightly (go-gitea#32344)
  Fix broken image when editing comment with non-image attachments (go-gitea#32319)
  Fix disable 2fa bug (go-gitea#32320)
  Upgrade rollup to 4.24.0 (go-gitea#32312)
  Upgrade vue to 3.5.12 (go-gitea#32311)
  Make admins adhere to branch protection rules (go-gitea#32248)
  Prevent from submitting issue/comment on uploading (go-gitea#32263)
  Add warn log when deleting inactive users (go-gitea#32318)
  Add `DISABLE_ORGANIZATIONS_PAGE` and `DISABLE_CODE_PAGE` settings for explore pages and fix an issue related to user search (go-gitea#32288)
  ...
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 31, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
b594773 
* giteaofficial/main:
  Fix suggestions for issues (go-gitea#32380)
  refactor: remove redundant err declarations (go-gitea#32381)
  Fix the missing menu in organization project view page (go-gitea#32313)
  Fix toAbsoluteLocaleDate and add more tests (go-gitea#32387)
  Respect UI.ExploreDefaultSort setting again (go-gitea#32357)
  Fix absolute-date (go-gitea#32375)
  Fix undefined errors on Activity page (go-gitea#32378)
  Add new [lfs_client].BATCH_SIZE and [server].LFS_MAX_BATCH_SIZE config settings. (go-gitea#32307)
  remove unused call to $.HeadRepo in view_title template (go-gitea#32317)
  Fix clean tmp dir (go-gitea#32360)
  Optimize branch protection rule loading (go-gitea#32280)
  Suggestions for issues (go-gitea#32327)
  Migrate vue components to setup (go-gitea#32329)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lunny lunny lunny approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.
5 participants
@6543 @confusedsushi @lunny @wxiaoguang @GiteaBot