https://pact.io logo
Join Slack
Powered by
Hey hey has it been spotted that 10.1.0 (similar f...
# pact-js
a
Hey hey has it been spotted that 10.1.0 (similar for all 10.x.x releases) will publish verification results even when 
publishVerificationResult
is false panic kermit ?
m
Umm...no I hope not!
y
Hey, I can recreate that. I don’t think the option is being passed from pact-js-core to the rust verifier https://github.com/pact-foundation/pact-js-core/blob/c4f60f2e9a4955d744e457c8d47245c3776af2a3/src/verifier/nativeVerifier.ts
m
y
should `opts.publishVerificationResult || false`be set here I would have assumed it would default to not publishing if no value provided
Copy code
if (brokerUrl && opts.provider) {
    ffi.pactffiVerifierBrokerSourceWithSelectors(
      handle,
      brokerUrl,
      opts.pactBrokerUsername || process.env.PACT_BROKER_USERNAME || '',
      opts.pactBrokerPassword || process.env.PACT_BROKER_PASSWORD || '',
      opts.pactBrokerToken || process.env.PACT_BROKER_TOKEN || '',
      opts.enablePending || false,
      opts.includeWipPactsSince || '',
      opts.providerVersionTags || [],
      opts.providerBranch || '',
      opts.consumerVersionSelectors
        ? objArrayToStringArray(opts.consumerVersionSelectors)
        : [],
      opts.consumerVersionTags || []
    );
  }
opts.publishVerificationResult
isn’t passed into when we call 
pactffiVerifierSetPublishOptions
Copy code
// TODO: extract these options into its own subtype, and check keyof
  if (
    opts.publishVerificationResult ||
    opts.providerVersion ||
    opts.buildUrl ||
    opts.disableSslVerification ||
    opts.timeout ||
    opts.providerVersionTags
  ) {
    ffi.pactffiVerifierSetPublishOptions(
      handle,
      opts.providerVersion || '',
      opts.buildUrl || '',
      opts.providerVersionTags || [],
      opts.providerBranch || '',
      
    );
  }
I’ll get a repro 👍 and will take a look
catjam 1
I am going to have to get pact-js-core running locally now, tried to avoid it yesterday, but thankful to Tim for his yak shaving yesterday on getting it running on m1 machines, as I think he has cracked it for me Thanks for the report @Adam Witko - happy for you to raise an issue
a
Raise it over in pact-js, yeah?
y
yeah pact-js please
✅ 1
danke
m
ok ignore this
When traveling to Boston, I did a whole refactor of the argument mapping for the FFI. It was beautiful, it was great, it was merged into master……..not
😄 2
😅 1
When I went back in just now and saw the code, I was a little confused. And realised I never merged it (probably because I was on a long haul flight without wifi and so couldn’t get a GH build to verify said work)
I’ve just rebased on master - because of all of the lovely contributions over the past few days - and this PR should have the fix: https://github.com/pact-foundation/pact-js-core/pull/391
(as well as improved validation)
It’s quite possible this will result in some upstream issues in Pact JS, because it does some additional validation on the arguments that may not have been there before. But they should be useful validations, catching potentially hidden bugs in the config
👍 1
y
😂 thanks matey! I might have a look at triggering an automatic build of pact-js with the updated pact-js-core dependencies on release, so save us having to manually update. I did take a look but couldn’t see anything already there
It was beautiful, it was great, it was merged into master……..not
This made me laugh 😂 we’ve all got this kind of story
😆 2
m
Gah! release failed because we don’t allow “refactor” as a changelog note. hmmm
y
yarrrg, @Timothy Jones this is one of those issues with the release process that irked me, I was trying to find you an example before I think a 
chore: fdfsd
or 
docs: foo
might work, if you update the release process docs 😅 (even tho the message says fix/feat) Or feat to bump a minor a version change in case the merge does cause further issues (which we want to iron out) I’ve never checked out to cause a breaking change bump in a commit message yet, I am going to go find out now 😅
ahhh nice just seen your addition to the package.json entry 👍
t
I really really want my contribution to pact-js to be the release notes practice. We can have really nicely written changelogs if you follow the guidelines in CONTRIBUTING ❤️
💯 2
Why do you want to release something that isn't a fix or feat?
y
This is good, nice one, you have left a good legacy 🙂 https://github.com/pact-foundation/pact-js/blob/master/CONTRIBUTING.md#release-notes The times I have come about it, is when I have been doing tweaks to CI pipelines and the npm builds, say to reduce the size of the compiled build, or npm-ignoring files. Refactors unless they aren’t breaking, wouldn’t allow a release as there wouldn’t be release notes generated, and the refactor might result in a smaller bundle.
Examples of 
fix
include bug fixes and dependency bumps that users of pact-js may want to know about.
Is there bug fixes/dep updates, we wouldn’t want users to know about? as it says may here. wonder what they would be classified as 
chore (deps):
?
It’s quite possible this will result in some upstream issues in Pact JS, because it does some additional validation on the arguments that may not have been there before. But they should be useful validations, catching potentially hidden bugs in the config
Hmm, so in this instance, would be want a minor bump on pact-js when consuming pact-js-core, but pact-js-core went out as a minor, which would maybe force consumers of it to think it wouldn’t have potential regressions. idk 😄
m
Why do you want to release something that isn’t a fix or feat?
It’s an important refactor, why shouldn’t I be able to release a refactor?
the refactor might add better performance, for example
Hmm, so in this instance, would be want a minor bump on pact-js when consuming pact-js-core, but pact-js-core went out as a minor, which would maybe force consumers of it to think it wouldn’t have potential regressions. idk
the types are the same, but the validation under the hood has been improved as has the way the properties are wired in. It’s a potential behavioural breaking change, but we don’t have a regression suite thorough enough to catch that sort of thing
So I’m just… staying vigilant batman
btw Tim, we have these in the notes that I wrote yeeeeeeeears ago:
Copy code
### Commit messages

Pact JS uses the [Conventional Changelog](<https://github.com/bcoe/conventional-changelog-standard/blob/master/convention.md>)
commit message conventions. Please ensure you follow the guidelines, as they
help us automate our release process.

You can take a look at the git history (`git log`) to get the gist of it.
If you have questions, feel free to reach out in `#pact-js` in our [slack
community](<https://pact-foundation.slack.com/>).

If you'd like to get some CLI assistance, getting setup is easy:

```shell
npm install commitizen -g
npm i -g cz-conventional-changelog
git cz
to commit and commitizen will guide you.``` I actually don’t use that anymore, but keen to know if you do?
t
Nope, I never have used that- I don't even have git aliases (no judgement to people who do- I have just never found them useful). On the “well, this refactor brings a performance increase”- great! Then it is a 
fix
, as it fixes something that users want to know about. The point of the commit message guidelines is to be intentional about what goes in the release notes. As a user, it's frustrating to read empty change logs if you're trying to work out what changed- so I forbade them in the release scripts.
👍 1
If there's no changes that users would want to know about, why are you releasing?
Similarly, the fix and feat commit messages have explicit guidelines, because it's more helpful to have the release notes have clear information on what changed from a user perspective, rather than a code perspective.
m
Yes in this case it just so happened a refactor fixed an underlying bug, but it was made for other reasons (there were numerous TODOs piling up that eventually got to me 🤣) I don't think we should avoid releasing code because there is no user facing impact for many of the same reasons you should practice continuous delivery, but I do think we should make it clear what is being released, and the wonderful improvements you made to that process help Commit messages are as much for maintainers as they are users of the software.
t
Yes. The thinking was to force the maintainer to make sure there are release notes, not to prevent releases. A few times I would make commits just for the release notes, which was annoying but still meant that the release notes were high quality
☝️ 1
Release-please solves this really nicely- it lets you override the release notes implied by the commits from a PR, which is great because you don't have to squash commits or ask a first time contributor to rewrite their messages
🤔 1
Blank release notes are pretty bad for trust when I'm looking at a new project
👍 1
m
I agree. I'll check out release please, that sounds handy for those times
t
Release please is awesome. It also won’t release if there would be no release notes 😉
❤️ 1
The way it works is it makes a PR with the changelog and bump commit, then keeps that PR up to date as more commits get added. Once the PR is merged, it tags that commit as the release and runs the scripts. This prevents the race condition problem that we had - and means releases won’t fail (as long as the build is repeatable and not brittle)
👍 1
Open in Slack
PreviousNext
Powered by