pact-foundation / pact-reference

Reference implementations for the pact specifications
https://pact.io
MIT License
91 stars 46 forks source link

Standalone Pact verifier doing extra unnecessary tests when verifying by URL and publishing #350

Closed gregtyler closed 5 months ago

gregtyler commented 6 months ago

I've struggled a bit to narrow down the cause of this completely, because it involves interacting with real services and publishing to our actual broker, but I think my interpretation is correct.

Problem

I have a provider with three consumers. I'm setting up a CI job that verifies a consumer's Pact, by its URL, against its provider (triggered from the contract_requiring_verification_published webhook). I want to publish these results back to the broker. Therefore I've ended up with a command like:

pact_verifier_cli --url={pact url} --publish

Understandably, the verifier needs to know where the broker is so that it can publish the results. So I add that argument (I've omitted auth here):

pact_verifier_cli --url={pact url} --publish --broker-url={broker url}

However, the verifier now verifies four pacts: the one provided with --url and the latest from each of the three consumers. As far as I know it only publishes verification for the --url one.

Verifying the extra pacts is a waste of time in CI, but it's also a potential blocker. Instead of pulling the latest pacts from the main branches of the consumers, it's getting them from the broker path /pacts/provider/{provider}/latest. This returns (and therefore it tries to verify) the absolute most recent of each consumer, even if they're from branches with invalid contracts. Thus the verifier may fail even though the one pact it was asked to check is fine.

Cause

I think the underlying issue here is that the --broker-url argument is being used for two things:

  1. Specifying where results publication should go (as a dependency of --publish)
  2. Identifying pacts to verify (as specified in the docs)

And so you end up verifying extra pacts just because you want to publish.

Specifically, this block of code seems to be where it's using the presence of the broker URL argument to go and fetch additional pacts.

Mitigation

I'm looking at a couple of fixes I can try to unblock us:

I also tried using nonsense --consumer-version-selectors ({"branch":"-invalid"}) so that it wouldn't return anything but, quite reasonably, that failed because it didn't return anything 😄

Potential fixes for pact_verifier_cli

(Assuming I've understood all of the above correctly!)

You could only fetch pacts from the broker if --consumer-version-selectors or --consumer-version-tags is provided (therefore removing this else clause), but that would be a breaking change. And presumably the else clause is there for good reason.

You could add a --publish-broker-url argument that is used for publication but doesn't trigger fetching additional pacts, but that feels verbose and easy to confuse.

You could add a --no-broker-fetch argument that explicitly opts out this behaviour when --broker-url is provided, which wouldn't be breaking but again feels verbose and a little unintuitive.

rholshausen commented 6 months ago

I've added a --webhook-callback-url option which will do what you need. You should be able to run it with

pact_verifier_cli --webhook-callback-url={pact url} --publish --broker-url={broker url} --provider-name {provider name}
gregtyler commented 6 months ago

@rholshausen Sorry for the slow response, but that seems to be working great, thank you very much!