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

webhooks: Add head_commit field to push payload. #5419

Closed
wants to merge 2 commits into from
Closed

webhooks: Add head_commit field to push payload. #5419

wants to merge 2 commits into from

Conversation

HarshitOnGitHub
Copy link
Contributor

Would appreciate comments/feedback on whether the approach taken is correct or not. Have tested the changes manually.

Fixes: #5266.

@HarshitOnGitHub HarshitOnGitHub mentioned this pull request Nov 27, 2018
Merged
@HarshitOnGitHub HarshitOnGitHub force-pushed the Issue_5266 branch 2 times, most recently from 965bd43 to 9da719c Compare November 29, 2018 18:13
@HarshitOnGitHub
Copy link
Contributor Author

(Updated dependency, build should pass now!)

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 29, 2018
@lafriks
Copy link
Member

lafriks commented Nov 29, 2018

Would be great if convert.ToCommit could be reused somehow to get api.PayloadCommit to not have so much duplicated code.

@lafriks lafriks added the type/enhancement An improvement of existing functionality label Nov 29, 2018
@lafriks lafriks added this to the 1.7.0 milestone Nov 29, 2018
@techknowlogick techknowlogick mentioned this pull request Dec 5, 2018
Closed
@lafriks
Copy link
Member

lafriks commented Dec 27, 2018

@HarshitOnGitHub can you fix to reuse convert.ToCommit?

@HarshitOnGitHub
Copy link
Contributor Author

@lafriks convert.ToCommit can't be used directly as it causes cyclic dependency. I was thinking of moving the function to some other place but couldn't decide on a good new place for it.

@HarshitOnGitHub
Copy link
Contributor Author

@lafriks Thoughts?

@techknowlogick techknowlogick modified the milestones: 1.7.0, 1.8.0 Jan 5, 2019
@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@af45648). Click here to learn what that means.
The diff coverage is 71.18%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5419   +/-   ##
=========================================
  Coverage          ?   37.76%           
=========================================
  Files             ?      327           
  Lines             ?    47874           
  Branches          ?        0           
=========================================
  Hits              ?    18080           
  Misses            ?    27195           
  Partials          ?     2599
Impacted Files Coverage Δ
routers/api/v1/repo/hook.go 19.15% <100%> (ø)
models/action.go 61.1% <69.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 af45648...2669ee4. Read the comment docs.

@HarshitOnGitHub
Copy link
Contributor Author

Any help/suggestions would be appreciated! :)

lunny
lunny requested changes Jan 19, 2019
View reviewed changes
models/action.go
@@ -720,6 +781,7 @@ func CommitRepoAction(opts CommitRepoActionOptions) error {
After: opts.NewCommitID,
CompareURL: setting.AppURL + opts.Commits.CompareURL,
Commits: opts.Commits.ToAPIPayloadCommits(repo.HTMLURL()),
HeadCommit: getHeadCommit(repo, opts.NewCommitID),
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 get the head PayloadCommit from APIPayLoadCommits ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be safe to assume that head commit is present at a specific location in the list(like at the starting or end) or are you suggesting to iterate over the list?

@techknowlogick techknowlogick modified the milestones: 1.8.0, 1.9.0 Feb 19, 2019
@stale
Copy link

stale bot commented Apr 20, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Apr 20, 2019
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Apr 20, 2019
@stale stale bot removed the issue/stale label Apr 20, 2019
@techknowlogick techknowlogick modified the milestones: 1.9.0, 1.10.0 Jun 4, 2019
@techknowlogick techknowlogick modified the milestones: 1.10.0, 1.11.0 Sep 3, 2019
@lunny
Copy link
Member

lunny commented Nov 16, 2019

Please resolve the conflicts

@lunny lunny modified the milestones: 1.11.0, 1.12.0 Dec 13, 2019
@lafriks lafriks modified the milestones: 1.12.0, 1.13.0 May 16, 2020
@lunny lunny modified the milestones: 1.13.0, 1.14.0 Sep 5, 2020
@lunny lunny modified the milestones: 1.14.0, 1.x.x Jan 27, 2021
@KN4CK3R KN4CK3R mentioned this pull request Jun 28, 2021
Merged
@6543 6543 closed this in #16282 Jun 29, 2021
@lunny lunny removed this from the 1.x.x milestone Aug 8, 2021
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny requested changes

Assignees
No one assigned
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

include head_commit in push webhooks if tag
6 participants
@HarshitOnGitHub @lafriks @codecov-io @lunny @techknowlogick @bkcsoft