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

[RFC] Change rules around automatic deploys, and add a CHANGELOG #1095

Closed
orta opened this Issue Aug 7, 2018 · 13 comments

Comments

Projects
None yet
3 participants
@orta
Copy link
Member

orta commented Aug 7, 2018

Proposal:

This proposes 2 changes:

  • Move Reaction to deploy on every PR
  • Enforce adding a user-facing CHANGELOG entry for Major and Minor changes

What the RFC aims to do is:

Deployment

  1. Someone makes a PR to Reaction
  2. They add one of the following labels: "Deploy: Patch", "Deploy: Feature" or "Deploy: Breaking".
  3. If they don't add a label, Peril will add "Deploy: Patch" for you (basically making it the default)
  4. When the PR is merged, the CI on master uses the GitHub API to determine which label is added and to deploy a version with that semantic version bump

CHANGELOG

A Danger rule can be added so that any PRs which are "Deploy: Feature" or "Deploy: Breaking" should include a user-facing changelog entry.

Reasoning

There are now 3 clients for Reaction. We want to increase visibility into what is changing, how and why. A list of PR titles are not useful enough to provide that kind of context in comparison to seeing what how useful the Emission changelog is for consumers and answering questions from product folk.

Exceptions:

There's a possibility that you could merge a PR without one of those labels, ideally that wouldn't happen but in that case I guess it would just not do a deploy.

Additional Context:

You can see our discussion in slack here and here

@damassi

This comment has been minimized.

Copy link
Member

damassi commented Aug 7, 2018

I very much like always having Reaction in a deployable state for the latest work which also means that rollbacks will be less jarring.

@orta

This comment has been minimized.

Copy link
Member Author

orta commented Aug 7, 2018

Worth noting, the method of asking for a changelog entries could be done via semantic release (which uses commit message formatting) - the tradeoff here is:

  1. We get changelog entries that can describe changes in a way that consumers of the library can understand rather
  2. Commit messages don't need any special formatting
@orta

This comment has been minimized.

Copy link
Member Author

orta commented Aug 7, 2018

Also, in Emission we grab the latest PR merged into master here

@zephraph

This comment has been minimized.

Copy link
Contributor

zephraph commented Aug 7, 2018

🍖

This feels right to me. Our current abstraction doesn't seem to really work well. We're using semantic-release to generate releases, but we're often creating dummy commits. I often don't know what changed since the last release some I'm uncertain if the bump should be minor, major, etc so it's hard to be true to semantic versioning.

One important note is that adopting this pattern means that we'll need to remove semantic-release. Being as it works on commit messages and this would be activated on merged commits with github labels, we'd need another mechanism for functionally triggering the release. I believe CircleCI has the ability to deploy artifacts, but barring that we might need to investigate something like release-it. I suppose the mechanism doesn't matter as much as the fact that the deployments actually happen on PR merge.

🥔 🥔

Deploy: Patch, Deploy: Feature, and Deploy: Breaking feel a little verbose to me. I get that it implies that a deployment will happen which provides good context for new people coming in to the project... it just feels like it wouldn't be completely necessary. I think if someone sees that every PR has a label and if danger either adds a label or fails if there is no label it would be evident enough that it's required. I suppose it's just a personal taste thing, but I'd rather it not have the Deploy: prefix. If you absolutely wanted a prefix to make the labels more explicit and grouped alphabetically then I'd prefer something relating to version which is more in line with what the labels semantically are.

@zephraph

This comment has been minimized.

Copy link
Contributor

zephraph commented Aug 8, 2018

@orta What about Release: Feature? It prefixes the labels with a common string (making them show up beside each other in the sort order) and explicitly states that it'll cause a version change. I think it'd be enough to infer from that that a release would be triggered.

@orta

This comment has been minimized.

Copy link
Member Author

orta commented Aug 8, 2018

Good for me, it has the clarity for what it does which is all I care about 👍

@orta

This comment has been minimized.

Copy link
Member Author

orta commented Aug 9, 2018

@zephraph had another idea, he thinks his final: Version: Feature - I'm still happy with that

@zephraph

This comment has been minimized.

Copy link
Contributor

zephraph commented Aug 9, 2018

Actually changed that to Release: Feature based on feedback from @sweir27.

@orta

This comment has been minimized.

Copy link
Member Author

orta commented Aug 10, 2018

Hah - I see

@zephraph

This comment has been minimized.

Copy link
Contributor

zephraph commented Aug 16, 2018

Hey @orta, had an idea related to this. After a new version of reaction is published, could we open a PR on all the consumers in the artsy org to bump the version? It could list out the changes since the last version in the PR.

That'd make version bumps on consumers much easier.

@orta

This comment has been minimized.

Copy link
Member Author

orta commented Aug 21, 2018

After a new version of reaction is published, could we open a PR on all the consumers in the artsy org to bump the version? It could list out the changes since the last version in the PR.

Yeah, maybe we could get greenskeeper hooked up on those repos for use @artsy/ deps also

@damassi

This comment has been minimized.

Copy link
Member

damassi commented Aug 21, 2018

@orta - One thing to note is that Greenkeeper has an ignore prop, but not an include prop; would we want to only update @artsy packages, or should we consider setting it up across the board? For applications it seems a bit dangerous, though for libs Greenkeeper seems ideal.

@zephraph zephraph self-assigned this Oct 13, 2018

@zephraph zephraph referenced this issue Oct 13, 2018

Closed

[WIP] Setup PR based release process #1407

0 of 5 tasks complete
@orta

This comment has been minimized.

Copy link
Member Author

orta commented Jan 3, 2019

This is now in production 🎉

@orta orta closed this Jan 3, 2019

@peril-staging peril-staging bot added the RFC label Jan 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment