PactBrokerWithSelectors ergonomic improvements

[jbwheatley]

Response to #81

Trying to keep the routes to illegal states at a minimum, while adding enough build options that the API isn't too cumbersome to use, especially in the case where settings are read from config. Illegal setting combinations will fail fast with an IllegalArgumentException explaining why the combination is illegal.

I've also turned on pending pacts by default, however see discussion on #69. This may not be the way to go.

Examples:

Good:

PactBrokerWithSelectors("brokerUrl")
  .withProviderTags(ProviderTags("MAIN"))
  .withWipPactsSince(WipPactsSince.instant(Instant.EPOCH))
  .withAuth(...)

PactBrokerWithSelectors("brokerUrl")
  .withPendingPacts(true)
  .withProviderTags(ProviderTags("MAIN"))
  .withAuth(...)

PactBrokerWithSelectors("brokerUrl")
  .withAuth(...)

Can fail:

val enabled: Boolean = ??? //read from config
val tags: List[String] = ???
val wipPactsSince: Option[LocalDate] = ???
PactBrokerWithSelectors("brokerUrl")
  .withPendingPacts(enabled)
  .withOptionalProviderTags(ProviderTags.fromList(tags)) //throws IllegalArgumentException if None and enabled=false
  .withWipPactsSince(WipPactsSince.optionalLocalDate(wipPactsSince)) // //throws IllegalArgumentException if Some(_) and ProviderTags.fromList(tags) is None
  .withAuth(...)

[jbwheatley]

moved to own file and up a namespace since it isn't just a field on PactBrokerWithSelectors anymore (also a setting in PublishVerificationResults

[solarmosaic-kflorence]

trying to understand the benefit of this over just using Option[Instant] 🤔 is it just that it allows consolidation of the logic for converting values to Instant in one place?

[jbwheatley]

it's this or have 6 different methods on PactBrokerWithSelectors. Felt like this solution gave nice guidance via types and method autocomplete

[solarmosaic-kflorence]

@jbwheatley definitely an improvement, thank you. I like the fail fast with exception approach, I think actually we can really simplify this whole interface if we do it that way. Instead of having all of these individual methods, we can just make a builder that gets verified on .build before the settings are actually used. That would allow retaining the normal ability to configure the settings in a flexible way with .copy while still getting validation.

[solarmosaic-kflorence]

Also, I think having pending enabled by default makes a lot of sense from a workflow perspective. The only danger there is that it differs from some of the other Pact library implementations, but I think we can probably fix that by pointing it out in the readme.

[TimothyJones]

Also, I think having pending enabled by default makes a lot of sense from a workflow perspective.

It does, but it means that you have to go all-in on pact. Enable pending moves the failure from the verification time, to the deploy check time.

I think the "best practice" is to use enable pending (and can-i-deploy to block merges to master + deployments). But I think the "common practice" might be to use the verification failure to do that instead, at least during pact setup.

[solarmosaic-kflorence]

@TimothyJones I've actually never used Pact without pending enabled, so maybe I don't fully understand, but I thought the two major changes when using pending vs not are that provider builds will not fail on features they don't explicitly opt-in to and the use of tags becomes a requirement (which perhaps also then requires the use of can-i-deploy).

So, without pending, all provider builds are essentially blocked by any non-compatible consumer updates? And they can only deploy once they satisfy those updates? I can't even 🧠 how deployments would work correctly for latest and previous consumer/provider versions without using can-i-deploy checks...

[jbwheatley]

When we eventually get set-up with pact4s, I don't imagine we'll be using pending pacts from the get go (our pact infra is very primitive). I guess I would err on the side of having features be opt-in, rather than opt-out, and disable it by default. So I guess I agree with @TimothyJones

[solarmosaic-kflorence]

I think the intent of the whole pending discussion is that the Pact foundation wants to make it a part of the default workflow, instead of a feature on top of the default workflow (leaving the option to disable it for teams that already have Pact integrations with it disabled). So the target is really new implementations. From my perspective, you will get a much healthier and easier to reason about Pact implementation if you use pending from the start, and I think it would be nice for Pact implementations to prefer funneling new implementations into best practices by default.

That said, I don't have a strong opinion on it being set to true by default at this time since the Pact foundation itself has decided not to do it yet.

[TimothyJones]

I can't even 🧠 how deployments would work correctly for latest and previous consumer/provider versions without using can-i-deploy checks...

If you're doing continuous deployment (as in, deploying every commit to main), you would:

• Publish pact and use can-i-deploy on the consumer, tagging the pact with main if deploy is successful
• Verify main on the provider.

In this scenario, using pending pacts would be require unnecessary infrastructure and additional steps.

[solarmosaic-kflorence]

@TimothyJones that still uses can-i-deploy. Wouldn't you also have to use can-i-deploy on the provider and deploy that first, prior to the consumer? Seems like a strange workflow as described.

[TimothyJones]

If main is continuously deployed, you don't have to use can-i-deploy on the provider, because passing verification means it's safe to deploy.

[jbwheatley]

merged as no major issues with the syntax changes. We can revisit the pending pacts on/off debate at a later date 😸