Skip to content

Webhooks for repo creation/deletion #1663

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
Merged
merged 4 commits into from
Sep 3, 2017
Merged

Webhooks for repo creation/deletion #1663

merged 4 commits into from
Sep 3, 2017

Conversation

ethantkoenig
Copy link
Member

@ethantkoenig ethantkoenig commented May 4, 2017 •
edited
Loading

Allow organizations to have webhooks that are triggered by repository creation/deletion. Partially addresses #2113.

Depends on go-gitea/go-sdk#55 (merged)
@ethantkoenig ethantkoenig mentioned this pull request May 4, 2017
Merged
@lunny lunny added the modifies/api This PR adds API routes or modifies them label May 4, 2017
lunny
lunny reviewed May 4, 2017
View reviewed changes
templates/repo/settings/hook_settings.tmpl Outdated
Copy link
Member

Choose a reason for hiding this comment

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

.Webhook.Repository?
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.
@lunny lunny added this to the 1.3.0 milestone May 4, 2017
@ethantkoenig ethantkoenig mentioned this pull request May 4, 2017
Closed
@ethantkoenig
Copy link
Member Author

Rebased to resolve conflicts.
@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 6, 2017
@ethantkoenig ethantkoenig force-pushed the etk/org_webhook branch 2 times, most recently from 0bfaa20 to 66eebe8 Compare May 25, 2017 18:07
@ethantkoenig ethantkoenig mentioned this pull request Aug 20, 2017
Closed
13 tasks
@stephenc
Copy link

Will be great to see this merged so I can add support to the https://github.com/jenkinsci/gitea-plugin
lunny
lunny reviewed Aug 29, 2017
View reviewed changes
models/repo.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Why not go HookQueue.Add(repo.ID) 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.

Fixed
lunny
lunny requested changes Aug 29, 2017
View reviewed changes
models/repo.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This will result database table lock on sqlite since we have a transaction here and another in PrepareWebhooks. It seems you have to create a prepareWebhooks(e Engine... method to avoid that.
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed
@lunny
Copy link
Member

lunny commented Aug 30, 2017

@ethantkoenig Still fail in sqlite on function prepareWebhook when call CreateHookTask.
@ethantkoenig
Copy link
Member Author

@lunny Good catch, fixed
@lunny
Copy link
Member

lunny commented Aug 30, 2017

@ethantkoenig but maybe you forgot to replace CreateHookTask to createHookTask?
lunny
lunny requested changes Aug 30, 2017
View reviewed changes
models/webhook.go
return prepareWebhook(x, w, repo, event, p)
}

func prepareWebhook(e Engine, w *Webhook, repo *Repository, event HookEventType, p api.Payloader) error {
Copy link
Member

Choose a reason for hiding this comment

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

Missing HookEventRepository on switch case. And missing handle on GetSlackPayload and GetDiscordPayload.
Copy link
Member Author

Choose a reason for hiding this comment

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

Added missing case to HookEventRepository. Will hopefully get change to add handles to GetSlackPayload and GetDiscordPayload tomorrow.
Copy link
Member Author

Choose a reason for hiding this comment

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

@lunny Done
@lunny lunny added type/feature Completely new functionality. Can only be merged if feature freeze is not active. and removed modifies/api This PR adds API routes or modifies them labels Aug 30, 2017
This was referenced Sep 1, 2017
Closed
Open
@lunny
Copy link
Member

lunny commented Sep 3, 2017

LGTM
@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 3, 2017
@lafriks
Copy link
Member

lafriks commented Sep 3, 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 3, 2017
@lafriks lafriks merged commit b689bb6 into go-gitea:master Sep 3, 2017
@stephenc
Copy link

stephenc commented Sep 3, 2017

(party) - when can we expect a release?
@lunny
Copy link
Member

lunny commented Sep 3, 2017

This is a part of v1.3 should be released 2017.10.25
@lunny
Copy link
Member

lunny commented Sep 3, 2017

@ethantkoenig maybe add rename webhook
@lunny
Copy link
Member

lunny commented Sep 3, 2017

And the option should be only when an orgnization webhook, when create/edit a repository webhoo, it should be hidden.

Repository Repository created or deleted
@ethantkoenig ethantkoenig deleted the etk/org_webhook branch September 9, 2017 23:40
@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.

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.

5 participants

@ethantkoenig @stephenc @lunny @lafriks @tboerger