#194 Add new generic webhook
Closed 4 years ago by praiskup. Opened 4 years ago by praiskup.
copr/ praiskup/copr custom-package-webhook  into  master

No commits found

The new /package/<copr_id>/<uuid>/<package_name>/ webhook is bound
to concrete package name. This means that concrete package build
is submitted after calling this hook, without the need to specify
concrete upstream git repo (it might not even exist, if that was
e.g. svn repo and we implemented the custom build method #185).

While we are on it, de-duplicate some webhook code.

So, you are saying, we should have only one URL entry point, read out message type and provider from the message http headers (e.g. "X-Github-Event") and then call the appropriate parser for each message type of the given provider. Well, yes that could have worked but we already have two separate URL entry points for each provider (Github, Gitlab) and we will need to keep them. It's not such a big deal. We should read message type in the Github webhook method and parse out data appropriately based on the type and do the same thing in the Gitlab webhook. Always we need to fill the source_dict_update structure with some data as you have suggested. In the Github webhook case, the ping event should be ignored. Simply put, I would drop the idea of generic webhook and added two more package specific entry points (for each Github and Gitlab message handler). If you want another entry point that will be just for dumping stuff somewhere, then yes, that one can be of generic name (but it should not contain rebuild_package call as the one here). It doesn't help that you are implementing support for your custom method, adding new functionality for the existing SCM method, and also refactoring code - all at the same time. There should be really be three (focused) pull requests, not just a single one.

Sorry but I would expect words like "heuristic" to be used for e.g. genetic algorithms or some kind hard bin-packing problem with non-uniform shapes or really in other area than what do here.

I welcome this pull request but I think it should be split into three (or at least let's drop any refactoring for now).

Btw. the (generic) webhook should only accept POST requests because it changes state of the system (launches build).

If you want another entry point that will be just for dumping stuff somewhere, then yes, that one can be of generic name (but it should not contain rebuild_package call as the one here).

My comment there was not right. I misunderstood the code there. rebuild_package call of course is needed there to launch the build at all.

But really I think, we need to take things step-by-step here: I would start by adding package-specific routes to the existing webhooks so that the magic around clone url matching to locate the appropriate package is not needed. I think that would be a good start.

Btw. the (generic) webhook should only accept POST requests because it changes state of the system (launches build).

I don't understand this, this just complicates build-submit through webhook without needed payload.

But really I think, we need to take things step-by-step here: I would start by adding package-specific routes to the existing webhooks so that the magic around clone url matching to locate the appropriate package is not needed. I think that would be a good start.

I'm not interested in /gitlab/package/ and /github/package paths... That's not useful for me (I'll never need that..); I'm really motivated by the generic method, so I'm motivated to get generic hook. Would it OK to start with this? If you want to iterated on the existing paths, I'm OK to let that on you (this PR is not affecting the old paths).

according to https://en.wikipedia.org/wiki/Heuristic it doesn't seem to be that invalid... I was trying to get one "best effort" guessing place which will do the cut between valid/invalid requests. But as you wish ... :) it is just a comment.

I don't understand this, this just complicates build-submit through webhook without needed payload.

This was meant to be rather academically -- where GET shouldn't cause any side effects on the server (submit build in this case). This http aspect is new for me, I still think it is less convenient to enforce POST, but accepting, will be dropped. Thank you!

rebased onto 66f6a7abf4b394cdf75e2636241ed6b0d4761e19

4 years ago

Please take another look.

  • the POST removed
  • the webhook is now named /generic/
  • other webhooks are untouched for now
  • single-line, from-command-line-readable error responses

Please, rename the variable "SRPM_STORAGE_DIR" to just "STORAGE_DIR".

If I understood correctly you need this webhook for the custom method. Then, please, keep the same naming everywhere ("custom" being the preferred choice).

Why is this webhook here anyway when @package_name_required is used below. This route does not contain any package so it is unusable or am I missing something?

I would really like not to see those "to-be" long comments in the code.

again, I would much prefer the webhook to be called webhooks_package_custom, so I would be happy if you could rename it back. not sure what "flavor" is good for.

Let's switch to 'custom', ok.

The route exists to give clear error report to curl if the package name is ommitted.

Can you provide a unit test for the webhook?

Meh. I consider it a good culture to let future editors to understand the desire of original contributor. Can you please rephrase?

Let's double check -- do you really want to change config api in this PR?

The good culture is to say what the function does if necessary and describe its arguments if any. We can talk about some wider context outside of the code. I also do not understand what you are talking about there so please just avoid any confusing long comments that are not about the code in that particular function or method.

Ye, ok, actually 404 is not clear enough? This is very strange and actually very redundant way to around things.

Yup, I don't want to store some random data in something dedicated to store SRPMs. You can also create another storage dir but I would just go for STORAGE_DIR.

For me '404', 'PACKAGE_NOT_FOUND' (or different status) is much clearer then generic 404 (in html it is just worse to understand, since this will be much likely debugged in commandline). It is just for user comfort, not necessary thing though.

Comment rephrased, renamed the option to STORAGE_DIR, s/generic/custom. Unittest is coming tomorrow probably.

rebased onto fc36f3546b6515922c06421a3bcb0b8423dde366

4 years ago

rebased onto 77e224fde9514531a9b8afa4588d9cc5e6f5d1aa

4 years ago

1 new commit added

  • [frontend] add tests for custom webhook
4 years ago

Thank you for adding tests. Can you make them work under python3? Tests are now being invoked by python3 during builds (unless --without check is used).

rebased onto 46fa2d8

4 years ago

Should be done (some python3 tests are still failing, but those shouldn't be broken by this PR).

Pushed from commandline.

Pull-Request has been closed by praiskup

4 years ago
Metadata