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.

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

Implement Default Webhooks #4299

Merged
merged 7 commits into from
Mar 19, 2019
Merged

Conversation

aunger
Copy link
Contributor

@aunger aunger commented Jun 22, 2018

Partially implement #770. (This is only webhooks, not git hooks.)

  • Add "Default Webhooks" page in site admin UI.
  • Persist to the existing webhooks table, but store with RepoID=0 and OrgID=0.
  • Add a few handlers and templates, but mostly modify the existing ones to serve double-duty. I should point out that installations that have customized the webhooks templates (probably uncommon) will need to edit them to accomodate a slight change in the semantics of .BaseLink.
  • Upon repo creation, copy the set of default webhooks into the new repo.
  • A repo owner may delete or edit these webhooks if he doesn't like the defaults.

Partially implement go-gitea#770.
Add "Default Webhooks" page in site admin UI.
Persist to the existing webhooks table, but store with RepoID=0 and OrgID=0.
Upon repo creation, copy the set of default webhooks into the new repo.

Signed-off-by: Russell Aunger <[email protected]>
@codecov-io
Copy link

codecov-io commented Jun 22, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4299   +/-   ##
=========================================
  Coverage          ?   38.86%           
=========================================
  Files             ?      365           
  Lines             ?    51341           
  Branches          ?        0           
=========================================
  Hits              ?    19953           
  Misses            ?    28516           
  Partials          ?     2872
Impacted Files Coverage Δ
routers/admin/hooks.go 0% <0%> (ø)
models/repo.go 47.36% <0%> (ø)
routers/org/setting.go 0% <0%> (ø)
routers/routes/routes.go 83.44% <100%> (ø)
models/webhook.go 63.18% <21.56%> (ø)
routers/repo/webhook.go 1.73% <3.33%> (ø)

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 cac9e6e...32b3e68. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 22, 2018
@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jun 23, 2018
@lunny lunny added this to the 1.6.0 milestone Jun 23, 2018
@aunger
Copy link
Contributor Author

aunger commented Aug 14, 2018

It's been a while. Should I merge master into this branch now to make review easier, or is it best to wait?

@aunger
Copy link
Contributor Author

aunger commented Aug 14, 2018

I'm confused about why drone failed. Is it a drone bug, or something I did wrong?

@techknowlogick
Copy link
Member

@aunger let me re-trigger drone.

@techknowlogick
Copy link
Member

@aunger drone is all good now. I suggest asking in the #develop channel on discord for a maintainer to review this PR

@aunger
Copy link
Contributor Author

aunger commented Aug 23, 2018

Thanks for your help, @techknowlogick, but I've tried Discord a few times and don't get much response. Would you mind doing the review yourself?

@lunny, would you review this too? I see you hearted it. It's been languishing for two months now.

Thanks.

@lunny
Copy link
Member

lunny commented Aug 24, 2018

@aunger I will review and test it. but that maybe spend my some time.

@lunny
Copy link
Member

lunny commented Aug 24, 2018

LGTM

@bkcsoft bkcsoft 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 Aug 24, 2018
@techknowlogick techknowlogick modified the milestones: 1.6.0, 1.7.0 Sep 3, 2018
@aunger
Copy link
Contributor Author

aunger commented Sep 3, 2018

@ethantkoenig Mind reviewing this PR? This would turn one of your ufw customizations into an admin setting. https://github.com/unfoldingWord-dev/gogs/blob/a2489be2c7ab4d3d20838135bb1d538c2eb5f40c/models/repo.go#L1363

@ethantkoenig
Copy link
Member

@aunger I won't be able to, sorry.

@lafriks
Copy link
Member

lafriks commented Sep 26, 2018

Can default webhooks be stored to new table, so that this functionality could be later extended more easily?

@aunger
Copy link
Contributor Author

aunger commented Sep 26, 2018

@lafriks Someone could certainly implement it that way. This PR's KISS approach is better in some ways though, because the plumbing is more easily shared (don't need an abstraction of a webhook if they are all the same) and it requires less attention to table schemas. What future extensions do you have in mind?

@lafriks
Copy link
Member

lafriks commented Sep 26, 2018

@aunger I would like to have ability to have forced webhooks for all repositories, updating webhooks for repositories when changing default webhook, ability to select/deselect from default webhooks in repository settings etc

@aunger
Copy link
Contributor Author

aunger commented Sep 26, 2018

@lafriks Great ideas. Site-wide hooks are more flexible. I was weighing most of that before I implemented this, but I didn't get any feedback (#770 (comment)), so I decided to go simple ("agile"?) with this implementation, because it's all my organization needs right now (and because it's what #770 explicitly asked for).

@aunger
Copy link
Contributor Author

aunger commented Sep 27, 2018

@lafriks I won't tackle site-wide hooks right now. If you're going to code it up now, that's awesome. If not, I hope you consider merging this implementation in the meantime. Thanks.

@techknowlogick techknowlogick modified the milestones: 1.7.0, 1.8.0 Dec 19, 2018
@techknowlogick techknowlogick modified the milestones: 1.8.0, 1.9.0 Feb 19, 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 Feb 19, 2019
@techknowlogick techknowlogick merged commit b34996a into go-gitea:master Mar 19, 2019
@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.
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
Development

Successfully merging this pull request may close these issues.

9 participants