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

Add versionist config #33

Merged
2 commits merged into from
Jul 27, 2017
Merged

Add versionist config #33

2 commits merged into from
Jul 27, 2017

Conversation

brownjohnf
Copy link
Contributor

Change-Type: patch

@brownjohnf brownjohnf requested a review from hedss July 11, 2017 21:54
@brownjohnf
Copy link
Contributor Author

I'm confused because this repo seems to already be versioned, but I don't see any versionbot integration.

@Page-
Copy link
Contributor

Page- commented Jul 11, 2017

Is a versionist.conf.js needed? What differences do we want from the default?

Also this is versioned manually currently, it's from long before versionist existed

@brownjohnf
Copy link
Contributor Author

You have to use a versionist config any time there's no package.json in the repo.

Copy link
Contributor

@hedss hedss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs the CHANGELOG.md file to be updated to use the standard header:

# Change Log

All notable changes to this project will be documented in this file
automatically by Versionist. DO NOT EDIT THIS FILE MANUALLY!
This project adheres to [Semantic Versioning](http://semver.org/).

parseFooterTags: true,
getGitReferenceFromVersion: 'v-prefix',
incrementVersion: 'semver',
updateVersion: (cwd, version, cb) => { cb(); },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's actually now a better way of doing this, removing this line and just adding the option:

  editVersion: false,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

module.exports = {
// This setup allows the editing and parsing of footer tags to get version and type information,
// as well as ensuring tags of the type 'v<major>.<minor>.<patch>' are used.
// It increments in a semver compatible fashion and allows the updating of NPM package info.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...except this particular config doesn't update NPM package info ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


var getAuthor = (commitHash) => {
return execSync(`git show --quiet --format="%an" ${commitHash}`, { encoding: 'utf8' }).replace('\n', '');
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for using this here, when the 'upstream' versionist.conf.js does:

const execSync = require('child_process').execSync;

const getAuthor = (commitHash) => {
  return execSync(`git show --quiet --format="%an" ${commitHash}`, {
    encoding: 'utf8'
  }).replace('\n', '');
};

IMHO if any differences can be minimised, that'd be ideal.
At the moment, I'm aware of resin-base, edge-node-manager and leech all using their own versionist.conf.js files. See also product-os/versionist#84

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. I've just been copy/pasting this file around from repo to repo for a while now.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so I guess you were probably copy'n'pasting round an old version of a variant of versionist's versionist.conf.js, and the 'upstream version' of versionist.conf.js has since been updated. And I guess this will continue to be a problem for any resin.io projects that are using versionist / VersionBot but aren't using NPM, so I wonder if product-os/versionist#84 needs to be expanded into a more flexible long-term solution...?

@lurch
Copy link

lurch commented Jul 13, 2017

Is a versionist.conf.js needed? What differences do we want from the default?

See my suggestion in product-os/versionist#84 (comment)

@lurch
Copy link

lurch commented Jul 14, 2017

This also needs the CHANGELOG.md file to be updated to use the standard header

Given that we're mandating the same standards across all resin.io repos, is that something else that versionist or VersionBot could check?

@hedss
Copy link
Contributor

hedss commented Jul 14, 2017

@lurch You know where the issues list is... :)

@lurch
Copy link

lurch commented Jul 14, 2017

product-os/versionist#90

Change-Type: patch
@ghost
Copy link

ghost commented Jul 24, 2017

VersionBot failed to commit a new version to prepare a merge for the above pull request here: #33. The reason for this is:
Cannot read property 'split' of undefined
Please carry out relevant changes or alert an appropriate admin.

@brownjohnf
Copy link
Contributor Author

@hedss this is a weird error:

VersionBot failed to commit a new version to prepare a merge for the above pull request here: #33. The reason for this is:
Cannot read property 'split' of undefined
Please carry out relevant changes or alert an appropriate admin.

@hedss
Copy link
Contributor

hedss commented Jul 27, 2017

@brownjohnf I think I know what this is, the versioning doesn't have v prefixes. I'm going to create a new tag with this version and hopefully it should work.

@ghost ghost merged commit c41306e into master Jul 27, 2017
@ghost ghost deleted the versionbot branch July 27, 2017 15:54
@hedss
Copy link
Contributor

hedss commented Jul 27, 2017

@brownjohnf Yep, twas the tag. Merged, all good.

This pull request was closed.
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.

4 participants