pact-foundation / pact-jvm

JVM version of Pact. Enables consumer driven contract testing, providing a mock service and DSL for the consumer project, and interaction playback and verification for the service provider project.
https://docs.pact.io
Apache License 2.0
1.07k stars 475 forks source link

Add support for new 'pacts for verification' endpoint #942

Closed bethesque closed 4 years ago

bethesque commented 4 years ago

See master issue here: https://github.com/pact-foundation/pact_broker/issues/307

bethesque commented 4 years ago

Copy paste from slack conversation:

Beth (Pactflow.io) 9:48 AM Here's the issue, but the details are in the Pact Broker repo, because I need to open one issue in each of the repos, and didn't want to have to copy/paste all that text each time.

I've never worked on pact-jvm, so I'm afraid I can't give you much guidance, but you can ask in the pact-jvm channel. You'll need to do some Kotlin, I believe. Having done a poke around to find the place where the pacts are currently being fetched, I believe it's these methods:

https://github.com/DiUS/pact-jvm/blob/master/core/pact-broker/src/main/kotlin/au/com/dius/pact/core/pactbroker/PactBrokerClient.kt#L93 https://github.com/DiUS/pact-jvm/blob/master/core/pact-broker/src/main/kotlin/au/com/dius/pact/core/pactbroker/PactBrokerClient.kt#L70

I'm not sure why the class is called PactBrokerConsumer, when it appears to me to be a Pact object. You'll need to find where those two methods are called, and turn those two separate calls into one single call to the new API.

data class PactBrokerConsumer @JvmOverloads constructor (
  val name: String,
  val source: String,
  val pactBrokerUrl: String,
  val pactFileAuthentication: List<String> = listOf(),
  val tag: String? = null
)

To me, the above class looks like a Pact, not a Consumer. It may need a refactor.

araknoid commented 4 years ago

Hi @bethesque , as discussed in the #994 , I'll work on this issue. Please let me know any pointers/hints that could help me start on this issue.

bethesque commented 4 years ago

My comment above is as much as I know in regards to the java code, sorry! Have a really good read of the master issue, and hop on to slack.pact.io and give me an @ to let me know you're there so we can discuss anything that's unclear. Ron should be able to give you pointers on the PactJVM code, but it would help if you had a poke around beforehand in your own IDE - I just worked out the stuff I've written in the comments by doing a few key word searches and following my nose through the code.

uglyog commented 4 years ago

FYI

data class PactBrokerConsumer @JvmOverloads constructor ( val name: String, val source: String, val pactBrokerUrl: String, val pactFileAuthentication: List<String> = listOf(), val tag: String? = null )

To me, the above class looks like a Pact, not a Consumer. It may need a refactor.

That class does not represent a Pact (that is in core/model module), it represents the config for the consumer side of the relationship in the broker. When executed against the broker, it results in the URL of a Pact.

bethesque commented 4 years ago

Oh, thanks Ron, that makes sense. It's a bit like what I've called a "selector" in the pact broker code and the docs for this issue then - a way to specify which pact(s) you want using some or all of consumer name/provider name/tags/scope, that later gets resolved to an actual pact url.

bethesque commented 4 years ago

@araknoid are you still keen to do this? Ron was looking at picking it up but wasn't sure if you'd started

araknoid commented 4 years ago

Hi @bethesque , I started looking/exploring the code but didn't have enough time to actually do anything. If someone else would like to take it, no problem (nothing will be lost 😉 ). Said so, If Ron will take this one, is there one that I could be working on as I should have enough time from today onwards? Thanks

mefellows commented 4 years ago

I can see some commits against this card @uglyog , what's outstanding for it to be completed?

uglyog commented 4 years ago

The base code is done, it just needs to be tested more

sab1l commented 4 years ago

Hi, @uglyog, please remember to keep us posted regarding any progress here or expected release. We're looking forward to use this feature since we're facing the very issue it should solve. Also we understand a proper testing may need time, so we'll wait whatever is needed.

uglyog commented 4 years ago

This has been released with version 4.1.0

bethesque commented 4 years ago

Closing as done! ❤️