pact-foundation/pact_broker-client

Support webhook upsert

bethesque opened this issue · 19 comments

bethesque commented

To support putting the webhook configuration in the codebase, rather than making it a once off manual action. Automate all the things.

bethesque commented

@jameshd can we get some background on this so we implement the Right Thing?

Would this webhook be created for consumers or providers or both?
Would it be once per consumer/provider ever or could there be potentially one per branch?

I can think of a couple implementations, given my current understanding of the requirements.

  1. Using uuid as unique key
    Currently webhooks are created by POST and a uuid is generated on the server. We could add a PUT handler that allowed a client generated uuid. A nice touch would be adding a generate-uuid command to the CLI for the initial configuration.

create-webhook --uuid 0eGVUsdC_i00zm08p3OgXQ --request POST ...

  1. Using name as unique key
    An alternative is to make the name the unique key. The client would need to check if a webhook with that name existed, and if it did, create/update appropriately. Has the potential to encounter race conditions.

The tricky thing with the ordering of the consumer/provider/webhook creation is that I'm not sure it can be 100% automated. I'm assuming the consumer upserts its "verification published" webhook when it publishes its pact, and the provider upserts its "pact published" webhook when it publishes its verification results.

If a new consumer build ran first, created its "verification results published" webhook, published the pact... then the provider build wouldn't automatically run because the provider's webhook wouldn't have been configured until it actually ran the verification step. Something has to get in there first and create the "pact published" webhook for the provider that first time.

jameshd commented

Hi Beth,

Thanks for looking at this. My original use case was to have our consumer's service template include the ability to set up a webhook in it's Jenkinsfile. So, out of the box when a new microservice is created the example pacts are in place, as is the webhook and the whole process "works out of the box".

I've worked around the issue by creating a webhook for all consumers contract_content_changed event which seems to be working well. However, I think it would still be nice to be able to specify a webhook per consumer, because the debugging process only shows the last run webhook in the logs and I can't specify which pact to run again as I cannot trigger the webhook with parameters, only empty POST body - unless I'm missing something there?

pact

I'd like to be able to specify which consumer / provider in the body - can I?

To address the original issue, I think specifying a name upfront or --uuid would be the best option, then that just overwrites the existing or creates the webhook. It shouldn't matter how many times the webhook is attempted to be created. There could also be an --ignore-if-existing flag to avoid overwriting perhaps?

bernardobridge commented

Hey Beth, I was one of the people asking for this while I was at DAZN. Here's my thoughts:

Would this webhook be created for consumers or providers or both?

I think both!

Would it be once per consumer/provider ever or could there be potentially one per branch?

I haven't come across the need of needing it on multiple branches, personally, so would be happy with the first.

I can think of a couple implementations, given my current understanding of the requirements.

  1. Using uuid as unique key
    Currently webhooks are created by POST and a uuid is generated on the server. We could add a PUT handler that allowed a client generated uuid. A nice touch would be adding a generate-uuid command to the CLI for the initial configuration.

create-webhook --uuid 0eGVUsdC_i00zm08p3OgXQ --request POST ...

I personally prefer this option!

The tricky thing with the ordering of the consumer/provider/webhook creation is that I'm not sure it can be 100% automated. I'm assuming the consumer upserts its "verification published" webhook when it publishes its pact, and the provider upserts its "pact published" webhook when it publishes its verification results.

If a new consumer build ran first, created its "verification results published" webhook, published the pact... then the provider build wouldn't automatically run because the provider's webhook wouldn't have been configured until it actually ran the verification step. Something has to get in there first and create the "pact published" webhook for the provider that first time.

I think one way to get around this (which requires the same implementation from the pact-broker, I think), would be to allow the implementations (like pactJS) to be configured with a webhook as well. Meaning, the first time a Pact is published using PactJS, it could first create the consumer provider pair (by publishing the pact), then create the webhook, and then trigger the webhook.

The disadvantage of that is of course having to implement it in each of the Pact implementations.

Just brainstorming!

Thanks for picking this up!!

shen-sat commented

Hi! I may be picking up this work (@bethesque directed me here) as I'm keen to contribute more to open source. I've made one contribution to OS before and I've worked with Pact before. But I should give a heads up that I'm new-ish to web development and I've never worked with webhooks so I might need a bit more context 😅 I hope that's cool! (If not, happy to work on another issue @bethesque 🙂 👍 )

Can I confirm here what the steps for a basic pact implementation are, and where the webhook fits in?

From my understanding/memory:
A consumer build is run -> a pact is generated and published to the pact broker
A provider build is run -> this in turn runs a pact verifier -> which in turn runs the provider tests against the pact broker -> and this will give a pass or fail

Where does the webhook fit into this? I don't remember having to set one up when I used Pact on a previous project.

From what I've read about webhooks so far, they work like an api but instead of needing a request before sending out a response, they simply give out responses (when certain criteria are met, I'm assuming). What responses are we sending out and what criteria are being met to trigger the sending of data from the webhook?

Apologies again for all the questions! I'm keen to help, just need to get my bearings :)

bethesque commented

Sorry for the slow reply @shen-sat. I've been on holidays.

Webhooks are pre-configured requests that are fired in response to events that happen in the Pact Broker, the most common ones being "pact changed" and "verification published".

Here are the webhook docs: https://github.com/pact-foundation/pact_broker/wiki/Webhooks

You can play around with a test broker here.

Broker base URL: https://test.pact.dius.com.au
username: dXfltyFMgNOFZAxr8io9wJ37iUpY42M
password: O5AIZWxelWbLvqMd8PkAVycBJh2Psyg1

Let's go with the uuid option.

Once this is done, you're welcome to have a go at implementing the code in the Pact Broker, or I can bash it out for you. It should be very little actual coding work.

shen-sat commented

Hi @bethesque, no worries at all and I hope you had a good holiday!

Thanks for the providing the steps for me to work on, I’ll start the work and shout if I get stuck 👍

Would you mind looking at my summary of this issue below? I understand some of the conversation above, but I want to make sure I have a complete picture of what I’m building (partly to help me implement the feature but also for my own learning 🙂 ):

Pact currently provides the user the option to create two types of webhook: contract_content_changed and provider_verification_published.

The issue (as @jameshd described) is that when a webhook (eg contract_content_changed) is fired, if there are multiple consumers in a service, then it is difficult to know which consumer changed the contract (and therefore triggered the webhook).

To solve this issue, we are going to allow the user to provide a uuid when they create a webhook for a consumer - that way, when a webhook is fired, the user can tell which entity (in this example, consumer) triggered it.

Does that make sense? And if so, is 'support webhook upsert' simply another way of saying 'support creating unique web hooks'?

Thanks again!

bethesque commented

The issue (as @jameshd described) is that when a webhook (eg contract_content_changed) is fired, if there are multiple consumers in a service, then it is difficult to know which consumer changed the contract (and therefore triggered the webhook).

No, sorry. The issue is that currently, creating a webhook is a once off manual step, and people would like this to be automated so that all the Pact related configuration can be specified in the code. The easiest way to support automation is to make the webhook creation step idempotent, so it can just be run every time a pact or verification is published. The easiest way to make webhook creation idempotent is to to support PUT.

Does that make sense?

  • 👍1
shen-sat commented

Ah gotcha! That makes sense sense, thanks for explaining. One thing I'm learning about open source is how quickly it levels you up in things you never knew about. Yesterday I didn't know about idempotency and how PUT relates to it; today, after your reply and a YT vid, I get it 🙌

A more user-focused question: do we want to make specifying a uuid optional? Ie if a user wants to manually create a webhook only once, they still can?

bethesque commented

Good question, yes we do want to keep the existing functionality as well. It's always best to backwards compatible whenever possible. So, the --uuid will be optional, and its presence will tell us whether we're doing the existing POST or the new PUT call.

  • 👍1
shen-sat commented

Makes sense!

Can I ask why we allow the user to specify the request type (I don't see why this is necessary, as currently we simply make a POST request regardless...?)

Would it make more sense to remove this so a user doesn't have to specify it? After all, we will decide whether to make a POST ot PUT request depending on whether a uuid has been provided or not (as you described), and therfore we can decide what to put here rather than the user?

bethesque commented

The request type is for the webhook that is being created, not the request that will be used to create the webhook. That is, when the webhook fires, it will use that type of request.

shen-sat commented

I see, thanks for the explanantion! I'll leave that part as is then

  • 👍1
bethesque commented

To make this more user friendly @shen-sat, I just had the thought - lets provide a new command like upsert-webhook or create-or-update-webhook. This will advertise the presence of the command to anyone reading the docs much better than toggling the behaviour based on the presence of a parameter. Any votes either way for those two options? I slightly lean towards create-or-update-webhook but could be persuaded the other way.

shen-sat commented

I agree we could make this functionality more explicit and I think create-or-update-webhook is more descriptive, so I vote for that!

Just to clarify: are we simply renaming the existing create-webhook URL command? Or are we keeping the create-webhook URL command and creating a new command?

bethesque commented

We are keeping the existing command, and adding a new create-or-update-webhook. It should have the same parameters as create-webhook, as well as a mandatory --uuid. I'm not sure if Thor (the CLI gem) will allow us to do any smart reuse of the parameter definitions, so just copy them for now, and we'll see if we can DRY them up later.

  • 👍1
bethesque commented

Released! Thanks for your work on this @shen-sat! OSS brownie points for you.

  • 👍1
shen-sat commented

Nice! I haven't got round to looking at the broker-side of the implementation, I'll try to do so soon.

It was cool working with you, thanks for the comments/reviews - I learnt loads!

ritikaarora692 commented

@shen-sat is there a way to create webhook through gradle? i seem to have similar situation where i would like to configure webhook if its not already present for a given pact.

bethesque commented

Not that I know of. You might be best calling the PUT API directly, given it's so easy to use the standard libraries directly from gradle tasks. Feel free to raise a request for it in the pact-jvm issues, but Ron's got a massive backlog of work at the moment, so if you're super keen to get it done, then it would be best to submit a PR for it.