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

feat: Allow webhooks match pacticipants by label #501

Conversation

barthez
Copy link
Contributor

@barthez barthez commented Sep 13, 2021

The idea behind this feature is to reduce repetetive webhooks for group of pacticipants that are groupped with the same label. If event and webhook url/body are the same it make hard to maintain multiple webhooks for set of pacticipants.

Add documentation and some failing specs for a feature of providing
pacticipants labels to webhooks so it can match subset of pacticipants
when triggered.
@barthez barthez force-pushed the make-webhooks-match-pacticipants-by-label branch from 68b0196 to ffaaf56 Compare September 13, 2021 14:23
@bethesque
Copy link
Member

Looking good!

@barthez
Copy link
Contributor Author

barthez commented Sep 15, 2021

Hey @bethesque ,
I think I'm ready with this PR. I tested it with following "reproduce-issue" script content:

  td.delete_integration(consumer: "Foo", provider: "Bar")
    .delete_integration(consumer: "foo-consumer", provider: "bar-provider")
    .create_pacticipant("Foo")
    .create_pacticipant("Bar")
    .create_pacticipant("Baz")
    .create_label("Foo", "the_label")
    .create_label("Baz", "the_label")
    .create_webhook_for_event(
      uuid: "cf51d68a-78b1-4164-912a-986096b2c3b3",
      event_name: ["contract_published", "provider_verification_published"],
      consumer: {label: "the_label"},
      url: "http://localhost:5011/status/200"
    )
    .publish_pact(consumer: "Foo", consumer_version: "1", provider: "Bar", content_id: "111", tag: "main")
    .publish_pact(consumer: "Baz", consumer_version: "1", provider: "Bar", content_id: "122", tag: "main")
    .get_pacts_for_verification(
      enable_pending: true,
      provider_version_tag: "main",
      include_wip_pacts_since: "2020-01-01",
      consumer_version_selectors: [{ tag: "main", latest: true }])
    .verify_pact(
      index: 0,
      provider_version_tag: "main",
      provider_version: "1",
      success: true
    )

And it triggers 3 webhooks, for both pacts publication and one verification publication.

I have one question though, should we show the webhook from preceding script on /webhooks/consumer/Foo endpoint?

@barthez barthez marked this pull request as ready for review September 15, 2021 21:01
@bethesque
Copy link
Member

I have one question though, should we show the webhook from preceding script on /webhooks/consumer/Foo endpoint?

Good question. I think not. Those endpoints are not exactly deprecated, but there's no real reason to use them for anything now. It's an endpoint designed to take a POST, assuming that the consumer will be the one in the path, and that is not the case for the label webhooks.

Copy link
Member

@bethesque bethesque left a comment

Choose a reason for hiding this comment

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

I've never had such a beautiful PR submitted :D Just that one little change please, and then I'm very happy to merge.

add_column(:consumer_label, String)
add_column(:provider_label, String)

add_constraint(:consumer_label_exclusion, "consumer_id IS NULL OR (consumer_id IS NOT NULL AND consumer_label IS NULL)")
Copy link
Member

Choose a reason for hiding this comment

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

Question - does this kind of constraint work on Sqlite and MySQL? I'm not too fussed if they don't, just curious.

Copy link
Contributor Author

@barthez barthez Sep 16, 2021

Choose a reason for hiding this comment

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

Base on https://dev.mysql.com/doc/refman/5.7/en/alter-table.html and https://dev.mysql.com/doc/refman/8.0/en/create-table-check-constraints.html check constraints are inefficient in mysql 5.x (syntax is accepted by engines ignore it). In 8.x adding named check constraints works in 8.0.16+, it is ignored prior that version.

Regarding SQLite, there seems to be an issue in Sequel adapter. I had to split alter_table block into 2 to handle adding both constraints at once. In other case it ended up adding last one only.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the info.

.maybe(:str?)
.when(:none?) { value(:name).filled? }

rule(label: [:name, :label]) do |name, label|
Copy link
Member

Choose a reason for hiding this comment

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

Well done, I've never managed to get the rules working for dry validation 😆 I find the gem very frustrating, and I'm locked on an old version because of a dependency from the roar gem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You cannot commit tears and blood to git repo, lot of these were used to pay for this to work 😄

Copy link
Member

Choose a reason for hiding this comment

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

You'll find many strongly worded comments from me scattered across the schema code, where I've just given up and done the validation manually 😆 I admire your persistence!

lib/pact_broker/api/decorators/webhook_decorator.rb Outdated Show resolved Hide resolved
@bethesque bethesque merged commit f30a9dc into pact-foundation:master Sep 17, 2021
@bethesque
Copy link
Member

Many thanks! I'll put out a release asap.

@bethesque
Copy link
Member

Out in 2.86.0.0

@barthez
Copy link
Contributor Author

barthez commented Sep 17, 2021

Thank you very much!

We will work on pact_broker-client upgrade next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants