feat(verify): support broker bearer token #147
Conversation
mefellows
force-pushed
the
feat/broker-bearer-token-support
branch
from
314836a to
4182866
Compare
mefellows
changed the title
| @@ -251,8 +253,7 @@ pact.verifyPacts({ | |||
| | Parameter | Required? | Type | Description | | |||
| | --------------------------- | --------- | ------- | ---------------------------------------------------------------------------------------------------------- | | |||
| | `providerBaseUrl` | true | string | Running API provider host endpoint. | | |||
| | `pactBrokerBaseUrl` | false | string | Base URL of the Pact Broker from which to retrieve the pacts. | | |||
| | `pactBrokerUrl` | false | string | URL of your Pact Broker to dynamically discover relevent pacts to verify. Required if `pactUrls` not given | | |||
There was a problem hiding this comment.
I've deprecated this field. So whilst it is still supported, I've removed it from the docs here and have provided a warning if it's used.
There was a problem hiding this comment.
I don't see why it should be deprecated. They do exactly the same thing no? Just the name that's different and I did that on purpose since base doesn't really add any value.
|
@mboudreau I thought about removing the Broker class entirely. We don't really document it, but technically it is still a public, usable API. If we want to keep with the idea that you can use a Node API to do certain pact things, then perhaps it's still worth keeping, otherwise I think we could make a case to remove it. |
|
@mefellows That looks pretty good. I'd remove the broker class if I were you; even if it's exported, that's not how the API should be used as it's meant to be an internal class. |
src/verifier.ts
Outdated
| @@ -68,12 +70,16 @@ export class Verifier { | |||
|
|
|||
| checkTypes.assert.nonEmptyString(options.providerBaseUrl, "Must provide the providerBaseUrl argument"); | |||
|
|
|||
| if (checkTypes.emptyArray(options.pactUrls) && !options.pactBrokerUrl) { | |||
| throw new Error("Must provide the pactUrls argument if no brokerUrl provided"); | |||
| if (options.pactBrokerUrl) { | |||
There was a problem hiding this comment.
I'm confused, why are we changing the name from pactBrokerUrl to pactBrokerBaseUrl?
There was a problem hiding this comment.
For consistency with all of the other languages and the CLI. Alternatively, I can just leave it as is and map to the new CLI arg?
There was a problem hiding this comment.
it was already pactBrokerUrl, leave it as is. No need to add another.
src/broker.ts
Outdated
| "user": this.options.username, | ||
| "password": this.options.password | ||
| }; | ||
| } else if (this.options.token) { |
There was a problem hiding this comment.
just create 2 ifs instead of having an else if. Might be good to type this function's return object like TokenAuth | UserPassAuth
There was a problem hiding this comment.
No probs. I'm going to remove the class as suggested.
|
@mefellows increment the version number to 7.1.0 |
|
Thanks for the feedback. I'm going to delete the broker class, map the field back to |
- Upgrade standalone to 1.64.0 - The CLI verifier has all of the logic needed to fetch pacts from the Broker. This change removes the use of the Broker class to fetch Pacts - Broker class removed as not required for public consumption - Adds support for bearer token authentication mode
mefellows
force-pushed
the
feat/broker-bearer-token-support
branch
from
4182866 to
f060b78
Compare
|
@mefellows only the minor version, not the major since you're not breaking the API if you don't change that argument name. |
from the Broker. This change removes the use of the Broker
class to fetch Pacts during verification, delegating to the underlying binary