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

Complete push webhooks #2530

Merged
merged 7 commits into from
Sep 21, 2017
Merged

Complete push webhooks #2530

merged 7 commits into from
Sep 21, 2017

Conversation

daviian
Copy link
Member

@daviian daviian commented Sep 17, 2017

Targets #2105 and thus #2418

Description:
PR enables push webhooks for branch and tag deletion to catchup with github's push webhook implemenation.
Furthermore it adds to new actions - ActionDeleteTag and ActionDeleteBranch.
Includes unit tests for CommitRepoAction function.

Needs to be rebased after #2459 is merged.

@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Sep 17, 2017
@lunny lunny added this to the 1.3.0 milestone Sep 17, 2017
@codecov-io
Copy link

codecov-io commented Sep 17, 2017 •
edited
Loading

Codecov Report

Merging #2530 into master will increase coverage by 0.24%.
The diff coverage is 42.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2530      +/-   ##
==========================================
+ Coverage   27.07%   27.32%   +0.24%     
==========================================
  Files          86       86              
  Lines       17119    17135      +16     
==========================================
+ Hits         4635     4682      +47     
+ Misses      11809    11775      -34     
- Partials      675      678       +3
Impacted Files Coverage Δ
models/update.go 11.17% <0%> (+0.47%) ⬆️
models/unit_tests.go 90.47% <100%> (+0.64%) ⬆️
models/action.go 54.28% <71.42%> (+4.17%) ⬆️
models/pull.go 20.58% <0%> (+0.38%) ⬆️
models/webhook.go 48.94% <0%> (+1.05%) ⬆️
models/repo_branch.go 33.33% <0%> (+21.21%) ⬆️

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 0d80af6...477ce63. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 17, 2017
ethantkoenig
ethantkoenig suggested changes Sep 18, 2017
View reviewed changes
modules/templates/helper.go Outdated
@@ -295,7 +295,7 @@ func ActionIcon(opType int) string {
switch opType {
case 1, 8: // Create and transfer repository
return "repo"
case 5, 9: // Commit repository
case 5, 9, 16, 17: // Commit repository
Copy link
Member

Choose a reason for hiding this comment

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

Could we please use the named constants (e.g. ActionDeleteTag) instead of magic numbers here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course.
Should it be done in this PR or should it be a separate one because it is not really part of this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it should be done in a new PR as it requires more changes in other parts of the code

models/action_test.go Outdated
s.commitRepoActionOptions.RepoName = repo.Name

s.action.ActUserID = user.ID
s.action.ActUser = user
Copy link
Member

Choose a reason for hiding this comment

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

nit: lines 315 and 317 aren't necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

Will remove the lines, It is based on the former implementation.

models/action.go
case ActionPushTag: // Create
isHookEventPush = true
Copy link
Member

Choose a reason for hiding this comment

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

This assignment has no effect, since if you reach here you will return on line 649.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above.

models/action.go
}); err != nil {
return fmt.Errorf("PrepareWebhooks: %v", err)
}
isHookEventPush = true
Copy link
Member

@ethantkoenig ethantkoenig Sep 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This assignment may have no effect, as you will return on line 626 if isNewBranch is true. I assume that you still want to fire the push webhook even if it is a new branch, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes correct. Forgot to replace the return by an error check.

@daviian
Copy link
Member Author

daviian commented Sep 18, 2017 •
edited
Loading

@ethantkoenig Implemented requested changes except changing int to ActionType as it requires more changes in other parts

@daviian daviian force-pushed the feature/complete-push-webhooks branch from f69d666 to c7f2b0e Compare September 18, 2017 22:35
@daviian
implemented missing 'delete' push webhooks
0bd387e 
moreover created ActionDeleteBranch and ActionDeleteTag

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>
@daviian
add CommitRepoAction tests for tag/branch creation/deletion
85cb006 
Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>
@daviian
fixed error where push webhook not called if is new branch or tag
8df0167 
removed unnecessary code

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>
@daviian
moved prepare unit test environment into separate method to be used a…
c64ea0c 
…cross unit tests

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>
@daviian daviian force-pushed the feature/complete-push-webhooks branch from c7f2b0e to c64ea0c Compare September 20, 2017 13:35
@daviian
add missing if clause in pushUpdate
8d2518d 
Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>
@daviian
Copy link
Member Author

daviian commented Sep 20, 2017 •
edited
Loading

Rebased current master as #2459 has been merged and has changed pushUpdate function as well

@ethantkoenig
Copy link
Member

LGTM

@ethantkoenig
Copy link
Member

Make L-G-T-M work

@tboerger tboerger 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 Sep 21, 2017
@lafriks
Copy link
Member

lafriks commented Sep 21, 2017

LGTM

@tboerger tboerger 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 Sep 21, 2017
@lafriks lafriks merged commit 1eedd98 into go-gitea:master Sep 21, 2017
@daviian daviian deleted the feature/complete-push-webhooks branch September 21, 2017 17:18
@daviian daviian mentioned this pull request Sep 21, 2017
Closed
7 tasks
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ethantkoenig ethantkoenig ethantkoenig 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/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.
6 participants
@daviian @codecov-io @ethantkoenig @lafriks @lunny @tboerger