Closed Kampfmoehre closed 3 years ago
This is an excellent report! Thank you for all the detail. I should hopefully have some time to look into this this weekend.
Is there a good reason for the pact url to be retuned in the promise? There could be multiple pacts published, so it's already buggy.
If you use --output json
and parse the response from the CLI, you'll be able to get the pact URLs (plural) out. You'll need to replicate the behaviour of the CLI output in node, which prints the notices to stdout using the following colours: https://github.com/pact-foundation/pact_broker-client/blob/2a51e37/lib/pact_broker/client/colorize_notices.rb#L21
The documentation for the response returned by the new "all in one" publishing endpoint is here: https://github.com/pact-foundation/pact_broker/blob/master/lib/pact_broker/doc/views/index/publish-contracts.markdown
It will only use the "all in one" endpoint that has the fancy response notices if it has a new version of the broker though. For old brokers, it will publish the old way (separate calls for each tag and each pact) and it doesn't support --output json
.
So you'd have to set --output json
, and try parsing to to JSON, and if it is JSON, print the notices, and if isn't, fall back to the current behaviour.
Is there a good reason for the pact url to be retuned in the promise?
Other than this is currently the interface, I don't think so. Pact-js doesn't actually even read the list, it just returns them to the user (which unfortunately means that changing it would be a breaking change).
There could be multiple pacts published, so it's already buggy.
I'm not sure I see the bug you're pointing out - my read of the current code is that it should work correctly if there are multiple URLs in the output (but also I can't see why this bug is happening either, so I'm clearly missing something).
If you use --output json and parse the response from the CLI,
Yes, I think we should definitely do this. Probably if we got the list of URLs from the JSON, it would also fix this bug.
(I believe this code pre-dates the ability to specify --json
, which is why it's parsing the output)
You'll need to replicate the behaviour of the CLI output in node
Hmm. I'm not super keen to replicate the CLI - I don't think it'll provide much value, and it'll make work for pact-js every time the CLI changes. Pact-js exposes the CLI to npm scripts anyway - the publish
part of the API is only for people building tools on top of pact.
I think a lot of people are still using their own custom publish script, because (until last week) the examples still used custom publish scripts (unnecessarily) instead of the CLI, even though that has been the easiest approach for a few years now - I don't think we highlighted the ability to use the CLI from npm scripts very well in the documentation.
@Kampfmoehre - do you have a particular use case for using the publishPacts
function instead of the CLI - or is this a case of "the documentation didn't explain that I could do that"?
Part of the reason is, we want to set up our version number correctly as well as the branch tag. This is integrated in our CI to run after Semantic Release, so we can't predict exactly what version the application has at this time. So we extract it from package.json (where Semantic-Release writes to). But there are other ways to do that (for example using jq on command line).
The other reason is, the script is some sort of legacy code that gets passed through every time we add a new project. I wanted to make this more common in our (GitLab) CI anyway so it works the same for .NET and NodeJS projects but haven't had the time so far. I will take a look at the CLI, I assume there is a container image for that?
You don't need a container image (although there is one) - pact-js exposes the CLI as bin stubs, so you can just use it direct in your npm script - see an example here:
"pact:publish": "pact-broker publish pact/pacts --consumer-app-version=\"$(npx @pact-foundation/absolute-version)\" --tag-with-git-branch --broker-base-url=https://test.pact.dius.com.au"
(technically pact-node/pact-core bring in the CLI, but those are dependencies of pact-js, depending on which version you have. You shouldn't need to install them separately).
I'd be keen to hear if you get a good pattern going with semantic release - it has so many opinions on how your build process should run that I reckon it's worth publishing a pact plugin for it.
I'm not sure if the pattern is good, but it works - for us at least. Since Pact is also part of the tests we run it two times. Of course we want to make sure tests pass before we make a release. For consumers this is not a problem since they work like unit tests and generate pact files. Provider tests don't offer that, so we run them in our test suite with publish = false. Both is just part of our test run - in CI as well as local. If tests are successful pact files are passed to the following jobs in the CI pipeline. Then Semantic Release is run (typically building and pushing a new container image. From this job updated files are also passed to following CI jobs. This includes the modified package.json. After that we publish pacts. Depending if the project has consumers or providers (many has both) two jobs are run. One just publishes the pact files via the script above, the other runs only the provider tests again with publish = true. After that we run deploy jobs that use the Pact CLI with pact-broker can-i-deploy and pact-broker create-version-tag
Now a Semantic Release plugin would be relatively easy for consumer pacts (given an earlier stage provides the pact files), but for provider pacts it would need to know what tests to run and how to run them.
I'm not sure I see the bug you're pointing out
I thought it returned a single URL, but you said "the list" Tim, so it must return multiple.
Hmm. I'm not super keen to replicate the CLI - I don't think it'll provide much value, and it'll make work for pact-js every time the CLI changes.
Very much agree.
We definitely want to encourage direct use of the CLI and not the JS wrappers that were there for historical reasons. JS code can always exec the binary if needed, and there are many other ways to get the needed values into a CLI.
I think we should seriously be considering deprecating the existing pact-core
APIs, because they all expose JS interfaces to the CLI which works against this principle.
I don't want this to become the next Maven/Gradle plugin problem!
We should probably still fix the exit code issue though, for our existing users!
Yes. We just got a separate issue that smells very similar.
Oh, I see the bug. It's because the regex is expecting the pact url at the start of the line, and the output no longer does that.
To fix quickly, we can remove the ^
part of the regex.
To fix properly, I think we should:
1) Use --json
and properly parse the output as Beth suggested
2) Not just throw all the output as the body of the error if we think there was an error (this meant that the unrelated permissions warning looked like it was the error) - can-i-deploy has a similar problem.
Other improvements:
1) Instead of emitting an error log for each line of the standard-error output, I propose we look for WARN
or warning
and emit warnings instead.
...also I have no idea how we put this bug into a release. This looks like it's tested to me, but again I must be missing something.
To fix quickly, we can remove the ^ part of the regex.
Yes
To fix properly, I think we should:
I though we agreed above not to be trying to replicate the CLI output? Ideally, it should print the stdout it gets, and the stderr it gets, and pass on the exit code.
Plus, we want to be now discouraging its use in preference of using the CLI directly.
This should start working again from pact-node v10.13.10
Software versions
Issue Checklist
Please confirm the following:
Expected behaviour
PactPublisher.publishPacts()
does not throw an errorActual behaviour
PactPublisher.publishPacts()
throws an errorSteps to reproduce
We run our own Pact Broker and use a Let's Encrypt cert for it. We had problems with it since the old cert is expired and used PACT_DISABLE_SSL_VERIFICATION=true until I saw we can update the pact-node package in #754 . We publish using a JS script wrapping it in a try/catch block. Publish seems successful (the message is indicating it and I can see it in Pact Broker) but the call to
publishPacts
somehow throws an error.Here is the code we use to publish pact:
According to the stacktrace the error is thrown here: https://github.com/pact-foundation/pact-js-core/blob/v10.13.9/src/publisher.ts#L121
It looks like the error contains the whole message, as you can see in the logs below the output is logged and then when we log the error thrown by
publishPacts()
it is all logged again.I have debugged a little bit and it seems that the regex is unable to extract the Pact Broker url. The string passed to it contains the whole message beginning with
opening connection to pact-broker.company.domain:443...
untilNo enabled webhooks found for the detected events
. I am not sure if the regex is wrong because it expects to match the url from the beginning of the string or if the wrong string is pased to the regex.Relevant log files