Skip to content

Allow creating and editing system webhooks. #23142

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

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions models/webhook/webhook_system.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,19 @@ func GetSystemWebhooks(ctx context.Context, isActive util.OptionalBool) ([]*Webh
Find(&webhooks)
}

// GetSystemOrDefaultWebhooks returns all admin webhooks.
func GetSystemOrDefaultWebhooks(ctx context.Context, isActive util.OptionalBool) ([]*Webhook, error) {
webhooks := make([]*Webhook, 0, 5)
if isActive.IsNone() {
return webhooks, db.GetEngine(ctx).
Where("repo_id=? AND org_id=?", 0, 0).
Find(&webhooks)
}
return webhooks, db.GetEngine(ctx).
Where("repo_id=? AND org_id=? AND is_active = ?", 0, 0, isActive.IsTrue()).
Find(&webhooks)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's improvements to be made here. For example, if this exists, do we really still need GetSystemWebhooks()? Would you ever use both?

// DeleteDefaultSystemWebhook deletes an admin-configured default or system webhook (where Org and Repo ID both 0)
func DeleteDefaultSystemWebhook(ctx context.Context, id int64) error {
return db.WithTx(ctx, func(ctx context.Context) error {
Expand Down
5 changes: 4 additions & 1 deletion modules/structs/hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ type Hook struct {
Events []string `json:"events"`
AuthorizationHeader string `json:"authorization_header"`
Active bool `json:"active"`
IsSystemWebhook bool `json:"is_system_webhook"`
Copy link
Member

Choose a reason for hiding this comment

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

Although the database stored as the name, but maybe we can have a better name like IsSystemOrDefault? Or even ScopeType?

Copy link
Contributor Author

@kousu kousu Jul 2, 2023

Choose a reason for hiding this comment

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

I've gotten around to this again finally.

I decided to replace IsSystemWebhook by a path parameter hookType:

gitea/routers/api/v1/api.go

Lines 1384 to 1390 in 5384c1d

m.Group("/{hookType:system-hooks|default-hooks}", func() {
m.Combo("").Get(admin.ListHooks).
Post(bind(api.CreateHookOption{}), admin.CreateHook)
m.Combo("/{id}").Get(admin.GetHook).
Patch(bind(api.EditHookOption{}), admin.EditHook).
Delete(admin.DeleteHook)
})

// swagger:operation GET /admin/{hookType} admin adminListHooks

// swagger:operation GET /admin/{hookType}/{id} admin adminGetHook

// swagger:operation POST /admin/{hookType} admin adminCreateHook

// swagger:operation PATCH /admin/{hookType}/{id} admin adminEditHook

// swagger:operation DELETE /admin/{hookType}/{id} admin adminDeleteHook

Doing this ballooned the PR, but it makes the API more friendly:

Screenshot 2023-07-01 at 23-41-57 Gitea API

and it also matches how the admin web UI works, where the two links on /admin/system-hooks/

Screenshot 2023-07-01 at 23-43-17 Gitea Git with a cup of tea

go to /admin/system-hooks/{kind}/new and http://localhost:3000/admin/default-hooks/{kind}/new respectively.

What do you think?

// swagger:strfmt date-time
Updated time.Time `json:"updated_at"`
// swagger:strfmt date-time
Expand All @@ -48,7 +49,8 @@ type CreateHookOption struct {
BranchFilter string `json:"branch_filter" binding:"GlobPattern"`
AuthorizationHeader string `json:"authorization_header"`
// default: false
Active bool `json:"active"`
Active bool `json:"active"`
IsSystemWebhook bool `json:"is_system_webhook"`
}

// EditHookOption options when modify one hook
Expand All @@ -57,6 +59,7 @@ type EditHookOption struct {
Events []string `json:"events"`
BranchFilter string `json:"branch_filter" binding:"GlobPattern"`
AuthorizationHeader string `json:"authorization_header"`
IsSystemWebhook *bool `json:"is_system_webhook"`
Active *bool `json:"active"`
}

Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/admin/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func ListHooks(ctx *context.APIContext) {
// "200":
// "$ref": "#/responses/HookList"

sysHooks, err := webhook.GetSystemWebhooks(ctx, util.OptionalBoolNone)
sysHooks, err := webhook.GetSystemOrDefaultWebhooks(ctx, util.OptionalBoolNone)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should change the behaviour? Maybe we should have parameter or we could have another endpoint for default webhooks?

if err != nil {
ctx.Error(http.StatusInternalServerError, "GetSystemWebhooks", err)
return
Expand Down
20 changes: 14 additions & 6 deletions routers/api/v1/utils/hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,12 +170,13 @@ func addHook(ctx *context.APIContext, form *api.CreateHookOption, ownerID, repoI
form.Events = []string{"push"}
}
w := &webhook.Webhook{
OwnerID: ownerID,
RepoID: repoID,
URL: form.Config["url"],
ContentType: webhook.ToHookContentType(form.Config["content_type"]),
Secret: form.Config["secret"],
HTTPMethod: "POST",
OwnerID: ownerID,
RepoID: repoID,
URL: form.Config["url"],
ContentType: webhook.ToHookContentType(form.Config["content_type"]),
Secret: form.Config["secret"],
IsSystemWebhook: form.IsSystemWebhook,
HTTPMethod: "POST",
HookEvent: &webhook_module.HookEvent{
ChooseEvents: true,
HookEvents: webhook_module.HookEvents{
Expand Down Expand Up @@ -324,6 +325,9 @@ func editHook(ctx *context.APIContext, form *api.EditHookOption, w *webhook.Webh
}
w.ContentType = webhook.ToHookContentType(ct)
}
if secret, ok := form.Config["secret"]; ok {
w.Secret = secret
}

if w.Type == webhook_module.SLACK {
if channel, ok := form.Config["channel"]; ok {
Expand Down Expand Up @@ -386,6 +390,10 @@ func editHook(ctx *context.APIContext, form *api.EditHookOption, w *webhook.Webh
return false
}

if form.IsSystemWebhook != nil {
w.IsSystemWebhook = *form.IsSystemWebhook
}

if form.Active != nil {
w.IsActive = *form.Active
}
Expand Down
1 change: 1 addition & 0 deletions services/webhook/general.go
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ func ToHook(repoLink string, w *webhook_model.Webhook) (*api.Hook, error) {
Type: w.Type,
URL: fmt.Sprintf("%s/settings/hooks/%d", repoLink, w.ID),
Active: w.IsActive,
IsSystemWebhook: w.IsSystemWebhook,
Config: config,
Events: w.EventsArray(),
AuthorizationHeader: authorizationHeader,
Expand Down
12 changes: 12 additions & 0 deletions templates/swagger/v1_json.tmpl

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.