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

System-wide webhooks #10546

Merged
merged 19 commits into from
Mar 8, 2020
Merged

System-wide webhooks #10546

merged 19 commits into from
Mar 8, 2020

Conversation

jamesorlakin
Copy link
Contributor

@jamesorlakin jamesorlakin commented Feb 29, 2020

#10545 was indeed too basic. This PR attempts to add webhooks that work system-wide, regardless of repository. The intent is to allow a savvy sysadmin to extend Gitea's functionality by being able to listen for all events - opened PRs/comments to add Jira cards to, deleting Docker images for the repo, etc. This also leaves default webhooks intact with a lot of code reuse but I fear it may be a tad confusing.

Tasks:

  • Create a new column to differentiate system webhooks from default ones
  • Adjust the router to serve both hooks and system-hooks reusing the same handlers
  • Tweak the creation of webhooks to determine if setting a value in the new column is necessary
  • Create modifications to the template and locales to explain the differences between the two
  • Fetch system webhooks and make use of them during events
  • Add API endpoints (will attempt in a separate PR)
    ...and probably something else

I'm more than happy to receive any feedback. This is my biggest PR so far but I'm growing quite fond of Go! 🚀

image

models/webhook.go Outdated Show resolved Hide resolved
models/webhook.go Outdated Show resolved Hide resolved
models/webhook.go Outdated Show resolved Hide resolved
models/webhook.go Outdated Show resolved Hide resolved
models/webhook.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 29, 2020
models/migrations/v128.go Outdated Show resolved Hide resolved
@jamesorlakin
Copy link
Contributor Author

@lafriks Oh yep! false and 0 should be different things... 👍

@jamesorlakin
Copy link
Contributor Author

Looking at what's currently there, there's no API for default webhooks. Should I look at trying to add that for both default and system webhooks (maybe another PR) or just leave everything as-is?

@lafriks
Copy link
Member

lafriks commented Mar 1, 2020

You can leave that for different PR. If code is too large it is harder to review and it takes more time to get it merged

@lafriks lafriks added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Mar 1, 2020
@lafriks lafriks added this to the 1.12.0 milestone Mar 1, 2020
@codecov-io
Copy link

codecov-io commented Mar 1, 2020

Codecov Report

Merging #10546 into master will decrease coverage by 0.07%.
The diff coverage is 8.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10546      +/-   ##
==========================================
- Coverage   43.78%    43.7%   -0.08%     
==========================================
  Files         586      587       +1     
  Lines       81764    81836      +72     
==========================================
- Hits        35798    35769      -29     
- Misses      41531    41623      +92     
- Partials     4435     4444       +9
Impacted Files Coverage Δ
models/migrations/migrations.go 4.16% <ø> (ø) ⬆️
routers/admin/hooks.go 0% <0%> (ø) ⬆️
models/migrations/v128.go 0% <0%> (ø)
routers/repo/webhook.go 1.24% <0%> (-0.04%) ⬇️
routers/routes/routes.go 86.27% <100%> (ø) ⬆️
models/webhook.go 66.98% <38.09%> (-1.73%) ⬇️
modules/webhook/webhook.go 43.88% <50%> (+0.27%) ⬆️
modules/indexer/stats/queue.go 62.5% <0%> (-18.75%) ⬇️
modules/indexer/stats/db.go 40.62% <0%> (-9.38%) ⬇️
services/pull/check.go 48.17% <0%> (-7.32%) ⬇️
... and 12 more

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 f9d34b2...cadb482. Read the comment docs.

@jamesorlakin jamesorlakin changed the title WIP: System-wide webhooks System-wide webhooks Mar 1, 2020
templates/admin/nav.tmpl Outdated Show resolved Hide resolved
@GiteaBot GiteaBot removed the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 2, 2020
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 2, 2020
routers/repo/webhook.go Outdated Show resolved Hide resolved
@lunny
Copy link
Member

lunny commented Mar 6, 2020

Please resolve the conflicts

@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 6, 2020
@lafriks
Copy link
Member

lafriks commented Mar 8, 2020

Make lgtm work

@lafriks lafriks merged commit a9f4489 into go-gitea:master Mar 8, 2020
@guillep2k
Copy link
Member

What about adding a changelog label?

@techknowlogick techknowlogick added the type/changelog Adds the changelog for a new Gitea version label Mar 8, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
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