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

@damassi => Add semantic-release #521

Merged
merged 8 commits into from
Feb 16, 2018
Merged

Conversation

eessex
Copy link
Contributor

@eessex eessex commented Feb 14, 2018

  • Adds semantic-release package
  • Adds custom commit message releaseRules to package.json in the Ember standard
  • Runs semantic-release after tests pass on master branch
  • Updates readme to include explanation and examples for commit formats.

GH_TOKEN has also been added to reaction on Circle CI (npm token was already present)

@eessex eessex requested a review from damassi February 14, 2018 18:52
@@ -27,6 +27,9 @@ test:
- yarn test -- --runInBand

deployment:
release:
branch: master
- yarn run semantic-release
Copy link
Member

Choose a reason for hiding this comment

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

I think there is some legacy stuff around here that relates to legacy deployment process. Once we've verified that this works we should make sure to remove that and update README.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure!

@damassi
Copy link
Member

damassi commented Feb 14, 2018

Talked with @eessex and we're going to setup a test repo to give the process a spin and figure out what we need and then come back here.

@eessex
Copy link
Contributor Author

eessex commented Feb 16, 2018

After merging conflicts, 17K lines were added to this PR -- any idea where these came from?

@orta
Copy link
Contributor

orta commented Feb 16, 2018

It looks like they came from the #522 change - I think you git add .'d all the old files, run find . -name __generated__ | xargs rm -rf, them yarn relay and then amend your commit 👍

@eessex eessex changed the title [WIP] Add semantic-release @damassi => Add semantic-release Feb 16, 2018
@@ -1,6 +1,6 @@
{
"name": "@artsy/reaction-force",
"version": "0.23.1",
"version": "0.0.0-development",
Copy link
Member

@damassi damassi Feb 16, 2018

Choose a reason for hiding this comment

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

Will this account for existing packages that have already been published to NPM? Does it determine from there that the current release is 0.23.1 and update accordingly?

Copy link
Contributor Author

@eessex eessex Feb 16, 2018

Choose a reason for hiding this comment

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

Yes, it should fetch the version number from the existing package on npm. This version number is a placeholder and will always read 0.0.0.

Copy link
Contributor Author

@eessex eessex Feb 16, 2018

Choose a reason for hiding this comment

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

When merging, the build on Circle will log which commits trigger changes, as well as the new version number.

Copy link
Member

Choose a reason for hiding this comment

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

Sweet, just wanted to double check 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also wanted to note it looks like this version number is being overwritten when I push a commit, but don't see where this is being set.

package.json Outdated
{"tag": "FEATURE", "release": "minor"},
{"tag": "BREAKING", "release": "major"}
]
}
Copy link
Member

Choose a reason for hiding this comment

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

👍

README.md Outdated
```

[Valid tags](https://github.com/artsy/reaction/blob/master/package.json#L175) for release include DOC, FIX (patch), FEATURE (minor), and BREAKING (major). Commits that do not adhere to this convention will not trigger an NPM release.

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about FIX vs PATCH as being the primary trigger? I always associate FIX with "BUGFIX" rathar than a minor change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to either, we can also add both or additional tags.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, lets go ahead and associate PATCH and FIX with a minor release

README.md Outdated
##### Example Patch Release
```
[FIX onboarding]: increase plus-button size
```
Copy link
Member

Choose a reason for hiding this comment

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

And update with additional example so that its clear:

[FIX onboarding] Modal does not open
[PATCH] Bump version 

Copy link
Member

Choose a reason for hiding this comment

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

Also, can you remove the : from the docs so that its clear that this syntax is not required?

@damassi
Copy link
Member

damassi commented Feb 16, 2018

This looks good to me -- @orta / @alloy, thoughts? Otherwise 👍

@orta
Copy link
Contributor

orta commented Feb 16, 2018

Have a think if there's a peril rule that could remind you about adding [FIX] or [PATCH] to commits

@damassi
Copy link
Member

damassi commented Feb 16, 2018

@orta - what I'm thinking is that those should only be added when a release is desired, though @eessex and I went back and forth about whether every commit should be a release. I'm in the camp that for a project like Reaction it should be targeted and selective. For example, there could be many commits in a PR that has nothing to do with a release, or there could be many commits and then the last one is tagged signalling a release, or the whole thing could be squashed into one commit with a tag. But I think it should be conventional rather than required. Thoughts? Maybe a danger warning? But even then, i think having our README up to date in terms of how to release is sufficient -- a git commit --allow-empty -m '[PATCH] would be enough.

@alloy
Copy link
Contributor

alloy commented Feb 16, 2018

I like it 👍

I like that you still need to trigger a release, the future will tell us wether or not people in practice run into issues with that.

@damassi
Copy link
Member

damassi commented Feb 16, 2018

Alright, gonna merge. Thanks for diving into this new area @eessex, will def help keep things efficient 👍

@damassi damassi merged commit 01910a9 into artsy:master Feb 16, 2018
@orta
Copy link
Contributor

orta commented Feb 16, 2018

Yeah, I like the idea 👍

@orta
Copy link
Contributor

orta commented Feb 16, 2018

i think it's simple enough to not need a danger/peril rule

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