fix(verifier): consumerVersionTag should support multiple tags #155
Conversation
|
Thanks for this @vgrigoruk. I'll need to trace this back as it seems strange/implausible that this option has gone this far and been ignored - i'd like to understand why/how. |
|
In pact-node v6 it was used to fetch URIs of pact contracts from a broker: https://github.com/pact-foundation/pact-node/blob/v6.21.4/src/verifier.ts#L151-L162 In pact-node 8+ this code is removed and resolving of these URLs is done in ruby code (pact-provider-verifier) |
|
Here is a commit where this code was removed: f060b78 |
|
Wow - quick response, and .. it was me! Yes this makes sense. Confusing naming perhaps that led to my confusion (tags makes more sense to me). This would be a backwards incompatible change and might be confusing for people? @mboudreau - what do you think about a deprecation notice in this case rather than a complete removal? Give it some time before fully removing? If not, could you please amend your commit by adding something like
This will ensure if we run a release on this commit it will bump the version appropriately. |
|
Also making a note that this will also require a change to pact-js (the verifier options are duplicated for annoying TypeScripty reasons). |
|
Considering that since f060b78 |
|
Yes please. I think we should consider that commit to be an error in that regard, so rather than two wrongs making a right we should mark it clearly. We can also manage it carefully from pact-js also. Thanks. |
| @@ -37,8 +37,7 @@ export class Verifier { | |||
| constructor(options: VerifierOptions) { | |||
| options = options || {}; | |||
| options.pactBrokerUrl = options.pactBrokerUrl || ""; | |||
| options.consumerVersionTag = options.consumerVersionTag || ""; | |||
| options.tags = options.tags || []; | |||
| options.consumerVersionTag = _.toArray(options.consumerVersionTag); | |||
There was a problem hiding this comment.
This will turn a string like "tag-1" into an array like [ 't', 'a', 'g', '-', '1' ]
| @@ -209,7 +209,7 @@ describe("Verifier Spec", () => { | |||
| }); | |||
| }); | |||
|
|
|||
| context("when consumerVersionTag is provided", () => { | |||
| context("when consumerVersionTag is provided as a string", () => { | |||
There was a problem hiding this comment.
This test needs to check the transformation of consumerVersionTag
| @@ -255,8 +255,7 @@ pact.verifyPacts({ | |||
| | `providerBaseUrl` | true | string | Running API provider host endpoint. | | |||
| | `pactBrokerUrl` | false | string | Base URL of the Pact Broker from which to retrieve the pacts. Required if `pactUrls` not given. | | |||
| | `provider` | false | string | Name of the provider if fetching from a Broker | | |||
| | `tags` | false | array | Array of tags, used to filter pacts from the Broker | | |||
| | `consumerVersionTag` | false | string | Retrieve the latest pacts with this consumer version tag | | |||
| | `consumerVersionTag` | false | string | array | Retrieve the latest pacts with this consumer version tag(s) | | |||
There was a problem hiding this comment.
The extra pipe denoting the union type here will actually try and create a new table column. Need to escape the | or problem just use or instead.
|
I'm going to bring this in @vgrigoruk, but gave some feedback on the PR so you can see what changes I'm going to make. Thank you for finding / addressing this. |
|
Going to pull in your changes in a separate branch with your commits. |
VerifierOptions.tagsis removed, as it is unused and it is not passed topact-provider-verifierVerifierOptions.consumerVersionTagsupports multiple tags (as per https://github.com/pact-foundation/pact-provider-verifier/blob/master/lib/pact/provider_verifier/cli/verify.rb#L21), so extendedpact-nodeto support this (for backward-compatibility, bothstringandstring[]are supported).