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

Move checks for pulls before merge into own function #19271

Merged
merged 15 commits into from Mar 31, 2022
Merged

Move checks for pulls before merge into own function #19271

merged 15 commits into from Mar 31, 2022

Conversation

6543
Copy link
Member

@6543 6543 commented Mar 30, 2022 •
edited

part of #9307

This make checks in one single place so they dont differ and maintainer can not forget a check in one place while adding it to the other .... ( as it's atm )

Fix:

  • The API does ignore issue dependencies where Web does not
  • The API checks if "IsSignedIfRequired" where Web does not - UI probably do but nothing will some to craft custom requests
  • Default merge message is crafted a bit different between API and Web if not set on specific cases ...

@6543 6543 added type/bug type/refactoring Existing code has been cleaned up. There should be no new functionality. labels Mar 30, 2022
@6543 6543 added this to the 1.17.0 milestone Mar 30, 2022
@6543 6543 mentioned this pull request Mar 30, 2022
Merged
19 tasks
@6543 6543 requested a review from wxiaoguang March 31, 2022 02:06
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 31, 2022
@wxiaoguang
Copy link
Contributor

It's marked as bug, what bug does it fix? Otherwise it looks good

@6543
Copy link
Member Author

6543 commented Mar 31, 2022 •
edited

@wxiaoguang

  • the API does ignore IssueDependencies where Web does not
  • the API checks if "IsSignedIfRequired" where Web does not - ui probably do but nothing will some to craft custom requests
  • Default merge message is craffted a bit different between API and Web if not set on specific cases ...

all hapens doue to copy code instead of having a single function for the job ...
... and with #9307 it would be three - so i took the chance to make it right

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 31, 2022 •
edited

@wxiaoguang

* the API does ignore IssueDependencies where Web does not

* the API checks if "IsSignedIfRequired" where Web does not - ui probably do but nothing will some to craft custom requests

* Default merge message is craffted a bit different between API and Web if not set on specific cases ...

all hapens doue to copy code instead of having a single function for the job

Wow, that's a lot, I think it's worth to mention them in the commit message (when merging) 😊

@6543
Copy link
Member Author

6543 commented Mar 31, 2022

pull desc updated

wxiaoguang
wxiaoguang reviewed Mar 31, 2022
View reviewed changes
routers/api/v1/repo/pull.go Outdated Show resolved Hide resolved
services/forms/repo_form.go Outdated Show resolved Hide resolved
services/forms/repo_form.go Outdated Show resolved Hide resolved
services/pull/protection_check.go Outdated Show resolved Hide resolved
wxiaoguang
wxiaoguang approved these changes Mar 31, 2022
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

LGTM (some non-blocker small nits)

@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 31, 2022
@6543
Copy link
Member Author

6543 commented Mar 31, 2022

@wxiaoguang added some proposed unrelated refactoring ...

@6543 6543 mentioned this pull request Mar 31, 2022
Merged
@6543 6543 requested a review from lunny March 31, 2022 13:29
@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 Mar 31, 2022
@6543 6543 changed the title Move checks for branchprotection into own package Move checks for pulls before merge into own function Mar 31, 2022
@6543 6543 merged commit 9c349a4 into go-gitea:main Mar 31, 2022
@6543 6543 deleted the single-place-to-enforce-protected-branch-rules branch March 31, 2022 14:53
6543 added a commit that referenced this pull request Mar 31, 2022
@6543
Move checks for pulls before merge into own function (#19271) (#19277)
123c254 
Backport #19271

Fix:
* The API does ignore issue dependencies where Web does not
* The API checks if "IsSignedIfRequired" where Web does not - UI probably do but nothing will some to craft custom requests
* Default merge message is crafted a bit different between API and Web if not set on specific cases ...
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 1, 2022
@zjjhot
Merge remote-tracking branch 'giteaoffical/main'
dd36434 
* giteaoffical/main:
  Fix broken of team create (go-gitea#19288)
  Remove `git.Command.Run` and `git.Command.RunInDir*` (go-gitea#19280)
  Performance improvement for add team user when org has more than 1000 repositories (go-gitea#19227)
  [skip ci] Updated translations via Crowdin
  Update JS dependencies (go-gitea#19281)
  Fix container download counter (go-gitea#19287)
  go.mod: update kevinburke/ssh_config to v1.2.0 (go-gitea#19286)
  Fix global packages enabled avaiable (go-gitea#19276)
  Add Goroutine stack inspector to admin/monitor (go-gitea#19207)
  Move checks for pulls before merge into own function (go-gitea#19271)
  Restore user autoregistration with email addresses (go-gitea#19261)
  Improve sync performance for pull-mirrors (go-gitea#19125)
  Refactor `git.Command.Run*`, introduce `RunWithContextString` and `RunWithContextBytes` (go-gitea#19266)
  Move reaction to models/issues/ (go-gitea#19264)
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. type/bug type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@6543 @wxiaoguang @lunny @GiteaBot