Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support branch/autoDetectVersionProperties when publishing pacts #408

Merged
merged 1 commit into from
Oct 31, 2022

Conversation

YOU54F
Copy link
Member

@YOU54F YOU54F commented Sep 28, 2022

Addresses pact-foundation/pact-js#749

Haven't added any tests in, but have tested with a local broker instance (spun up from the pact-workshop-js) repo

if autoDetectVersionProperties is true, but branch is set, it will use the provided branch value.
if autoDetectVersionProperties is true, but branch is not set, it will pick up the branch from my local machine
if branch is set, and is an empty string or undefined, no branch is set when uploading
if branch is set, is a valid string and autoDetectVersionProperties is not set to true, the user provided value is set

var pact = require('./src/pact');
var path = require('path');
const { Pact } = require('./src/pact');

var PactInstance = new Pact();

PactInstance.publishPacts({
  pactBroker: 'http://localhost:8000',
  pactBrokerUsername: 'pact_workshop',
  pactBrokerPassword: 'pact_workshop',
  pactFilesOrDirs: [
    path.resolve(__dirname, 'FrontendWebsite-ProductService.json'),
  ],
  consumerVersion: '1.0.3',
  branch: 'foo',
  autoDetectVersionProperties: true,
}).then(function (res) {
  console.log(res);
});

@bethesque
Copy link
Member

Is autoDetectVersionProperties working as you would expect?

@YOU54F
Copy link
Member Author

YOU54F commented Sep 29, 2022

Is autoDetectVersionProperties working as you would expect?

Well, it works how I expect it to behave, based on how I have interacted with the pact_broker-client, however that isn't to say, that is how one would possibly expect it to behave.

  1. Based on the output of setting the parameter autoDetectVersionProperties, we will automatically detect the branch. It feels like autoDetectBranchProperty would be more apt/intuitive
  2. As a user provided branch value takes precedence over the computed branch value determined if autoDetectVersionProperties is set to true, could we enable branch detection (and application) automatically, with the user having to opt out, or manually set a branch property? Rationale below
  • We want to get users to publish with branch set so they can use the new webhook event contract_requiring_verification_published
  • Users can override it being set, by passing the branch property
  • Users could opt out completely
  • It should cover the majority of use cases (is there a case where someone wouldn't want to publish with a branch )
  1. Is the ideal world of compatibility with our old docs/badges etc to advise publishing with branch and a tag representing the branch, or what would the potential implications be in applying a tag with the name on the branch automatically)
  • --auto-detect-version-properties --tag-with-git-branch

@bethesque
Copy link
Member

I think I meant the autoDetectVersionProperties to also do the version number, and the build URL, and just hadn't gotten around to doing them.

I wanted it to default to true, so it would just work without people having to do anything.

Is the ideal world of compatibility with our old docs/badges etc to advise publishing with branch and a tag representing the branch, or what would the potential implications be in applying a tag with the name on the branch automatically)

Nothing terrible would happen - it's just a bit messy. The Pact Broker already takes the first tag, and turns it into the branch name (unless someone disabled this config) to allow people to easily migrate.

@bethesque
Copy link
Member

I think I meant the autoDetectVersionProperties to also do the version number, and the build URL, and just hadn't gotten around to doing them.

Should I add in the version from the git sha?

@YOU54F
Copy link
Member Author

YOU54F commented Sep 30, 2022

That sounds sensible and looks like it was a feature req too

pact-foundation/pact_broker-client#100

@mefellows mefellows merged commit eb7c56e into master Oct 31, 2022
@mefellows mefellows deleted the support_publish_with_branch branch October 31, 2022 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants