Skip to content

Stop running hooks on pr merge #6963

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
Merged
merged 4 commits into from
May 17, 2019
Merged

Stop running hooks on pr merge #6963

merged 4 commits into from
May 17, 2019

Conversation

zeripath
Copy link
Contributor

Alternative fix for #6946

This just removes the final SSH_ORIGINAL_COMMAND preventing the hooks from being run whilst I figure out how to fix the issues in #6961

@zeripath zeripath added this to the 1.9.0 milestone May 15, 2019
@codecov-io
Copy link

codecov-io commented May 15, 2019 •
edited
Loading

Codecov Report

Merging #6963 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6963      +/-   ##
==========================================
- Coverage   41.47%   41.43%   -0.05%     
==========================================
  Files         441      441              
  Lines       59666    59667       +1     
==========================================
- Hits        24748    24723      -25     
- Misses      31696    31726      +30     
+ Partials     3222     3218       -4
Impacted Files Coverage Δ
models/helper_environment.go 85.71% <100%> (+0.71%) ⬆️
routers/private/push_update.go 20% <0%> (-24%) ⬇️
routers/private/repository.go 25.86% <0%> (-22.42%) ⬇️
models/unit.go 62.16% <0%> (-5.41%) ⬇️
models/update.go 49.24% <0%> (-3.02%) ⬇️
routers/repo/view.go 42.02% <0%> (-1.02%) ⬇️
models/repo_list.go 73.09% <0%> (+1.01%) ⬆️
modules/log/file.go 77.62% <0%> (+2.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04f996f...8f708b1. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 15, 2019
@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 May 16, 2019
@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 May 16, 2019
@zeripath
Copy link
Contributor Author

Hmm... the new unit test failure is very interesting. It doesn't appear to have anything to do with my changes.

@zeripath
Copy link
Contributor Author

In particular the failure is at:

panic: Failed to execute 'git config --global core.quotepath false': error: could not lock config file /root/.gitconfig: File exists
29

Has there been a change of the docker environment?

lunny
lunny requested changes May 17, 2019
View reviewed changes
models/pull.go Outdated
@@ -556,6 +556,8 @@ func (pr *PullRequest) Merge(doer *User, baseGitRepo *git.Repository, mergeStyle
}

env := PushingEnvironment(doer, pr.BaseRepo)
// FIXME: #6946 requires protected branches to be ignored
env = env[:len(env)-1]
Copy link
Member

Choose a reason for hiding this comment

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

This will assume that env is the last one. It's danger. I would like to traverse all the envs and remove the extract one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a longstanding fix it's just meant to last until I get #6961 fixed.

@zeripath
Copy link
Contributor Author

OK I've just removed the SSH_ORIGINAL_COMMAND from the pushing environment as I suspect that it's not working correctly in any case.

@lafriks lafriks requested a review from lunny May 17, 2019 11:05
@lunny
Copy link
Member

lunny commented May 17, 2019

But repo.OwnerName should still be repo.MustOwnerName()?

@zeripath
Copy link
Contributor Author

Yeah, any comprehensive fix will need that - could add it now if preferred.

@lafriks lafriks merged commit 96b412b into go-gitea:master May 17, 2019
@zeripath zeripath mentioned this pull request May 18, 2019
Closed
@zeripath zeripath deleted the fix-#6946-stop-running-hooks-on-pr-merge branch May 19, 2019 18:03
jeffliu27 pushed a commit to jeffliu27/gitea that referenced this pull request Jul 18, 2019
@zeripath @jeffliu27
Stop running hooks on pr merge (go-gitea#6963)
583f69d 
* Stop running hooks on pr merge

* Remove SSH_ORIGINAL_COMMAND from the pushing environment
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny approved these changes

@lafriks lafriks lafriks approved these changes

@mrsdizzie mrsdizzie mrsdizzie 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
Projects
None yet
Milestone
1.9.0
Development

Successfully merging this pull request may close these issues.
7 participants
@zeripath @codecov-io @lunny @lafriks @mrsdizzie @GiteaBot @techknowlogick