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

Decouple HookTask from Repository #17940

Merged
merged 24 commits into from
Oct 21, 2022
Merged

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Dec 9, 2021

At the moment a repository reference is needed for webhooks. With the upcoming package PR we need to send webhooks without a repository reference. For example a package is uploaded to an organization. In theory this enables the usage of webhooks for future user actions.

This PR removes the repository id from HookTask and changes how the hooks are processed (see services/webhook/deliver.go). In a follow up PR I want to remove the usage of the `UniqueQueue´ and replace it with a normal queue because there is no reason to be unique.

@KN4CK3R KN4CK3R added the type/enhancement An improvement of existing functionality label Dec 9, 2021
lunny
lunny previously requested changes Dec 10, 2021
models/repo.go Show resolved Hide resolved
models/webhook/hooktask.go Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 10, 2021
@lunny
Copy link
Member

lunny commented Dec 18, 2021

How about to add a new column named type which could be respository or package or user, and the repo_id could be changed to target_id?

@GiteaBot GiteaBot 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 Feb 17, 2022
models/repo.go Show resolved Hide resolved
@lunny lunny dismissed their stale review February 18, 2022 17:18

It should not be a block.

@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 22, 2022
@singuliere
Copy link
Contributor

@KN4CK3R could you please resolve the conflict?

models/webhook/hooktask.go Outdated Show resolved Hide resolved
services/webhook/webhook.go Outdated Show resolved Hide resolved
services/webhook/webhook.go Outdated Show resolved Hide resolved
@6543 6543 added this to the 1.17.0 milestone May 3, 2022
services/webhook/webhook.go Outdated Show resolved Hide resolved
@lunny
Copy link
Member

lunny commented Jun 3, 2022

Please resolve the conflicts

@6543
Copy link
Member

6543 commented Oct 17, 2022

@KN4CK3R please resolve conflicts

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Oct 17, 2022
@6543
Copy link
Member

6543 commented Oct 19, 2022

@silverwind silverwind removed the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Oct 19, 2022
@lunny
Copy link
Member

lunny commented Oct 21, 2022

Maybe I have posted this concern. When a hook task created from an org/system level webhook, if repo_id is removed from table hook_task, we cannot know which repository the hook_task belongs to easily.

@6543
Copy link
Member

6543 commented Oct 21, 2022

@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 Oct 21, 2022
@6543
Copy link
Member

6543 commented Oct 21, 2022

RepoID int64 `xorm:"INDEX"` // An ID of 0 indicates either a default or system webhook

@6543 6543 merged commit 1887c95 into go-gitea:main Oct 21, 2022
@lunny
Copy link
Member

lunny commented Oct 22, 2022

@lunny we do store RepoID in Webhook table: https://github.com/go-gitea/gitea/pull/17940/files#diff-8fd01247d233d732c2e8ffc0c554ad22dde28f35fc7e27f5489d5da39f53c79bR184

You don't really understand how webhook and hooktask work. A system/org level webhook's repo_id is always zero.

@KN4CK3R KN4CK3R deleted the feature-user-org-webhook branch October 22, 2022 11:07
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 24, 2022
* upstream/main:
  adapt README_{Country}.md stype name in localizedExtensions (go-gitea#21486)
  dump: Add option to skip index dirs (go-gitea#21501)
  Use recommended vscode configuration in gitpod environments (go-gitea#21537)
  Expand "Go to File" button again, fix 'Add File' margin (go-gitea#21543)
  Add yardenshoham to maintainers (go-gitea#21566)
  Refactor git command arguments and make all arguments to be safe to be used (go-gitea#21535)
  Update binding to fix bugs (go-gitea#21556)
  Link mentioned user in markdown only if they are visible to viewer (go-gitea#21554)
  Require authentication for OAuth token refresh (go-gitea#21421)
  CSS color enhancements (go-gitea#21534)
  Allow package version sorting (go-gitea#21453)
  Add link to user profile in markdown mention only if user exists (go-gitea#21533)
  Update milestone counters when issue is deleted (go-gitea#21459)
  Prevent Authorization header for presigned LFS urls (go-gitea#21531)
  Remove deleted repos from searchresult (go-gitea#21512)
  Remove unnecessary debug log (go-gitea#21536)
  Added check for disabled Packages (go-gitea#21540)
  Decouple HookTask from Repository (go-gitea#17940)
  Add color previews in markdown (go-gitea#21474)
  Fix generating compare link (go-gitea#21519)
lunny added a commit that referenced this pull request Nov 23, 2022
When re-retrieving hook tasks from the DB double check if they have not
been delivered in the meantime. Further ensure that tasks are marked as
delivered when they are being delivered.

In addition:
* Improve the error reporting and make sure that the webhook task
population script runs in a separate goroutine.
* Only get hook task IDs out of the DB instead of the whole task when
repopulating the queue
* When repopulating the queue make the DB request paged

Ref #17940 

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 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/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants