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

Chore: update release flow to use release PRs #285

Closed
erezrokah opened this issue Jan 13, 2021 · 7 comments
Closed

Chore: update release flow to use release PRs #285

erezrokah opened this issue Jan 13, 2021 · 7 comments

Comments

@erezrokah
Copy link
Contributor

erezrokah commented Jan 13, 2021

Our current release flow requires pushing directly to the default branch:
https://github.com/netlify/open-api/blob/master/CONTRIBUTING.md#making-a-new-release

We're in the process of migrating to release-please and should do the same for this repo.
See here and here.

The blocker for this issue is that we need to find a way to generate the go client (or avoid pushing auto generated files to the repo) during the process of creating the release PR.

@erezrokah
Copy link
Contributor Author

Some more context.
The release please action calls the release-please package programmatically:
https://github.com/google-github-actions/release-please-action/blob/88a82e1f9574f0a8bcacf1a13f84ef3d2c6659f3/index.js#L29

If we find the action too limiting we could write a script/our own action/fork the existing action and extend it.
Calling the release-please package programmatically will make it easier to manipulate the release PR.

@erezrokah
Copy link
Contributor Author

erezrokah commented Jan 15, 2021

We could also use the same approach as in:
https://github.com/netlify/cli/blob/491adc49cbc92d93ae0b720a0724fa1824ebb8a4/.github/workflows/verify-docs.yml#L29

To make sure people generate the go client when submitting a PR.

@JGAntunes
Copy link
Contributor

JGAntunes commented Jan 20, 2021

So, I've been looking into how this is tied together and I decided to break this issue into a couple of separate pieces of work, let me know what you think 😄

As it is right now, the versioning of the open-api spec is tied together with the go-client release - https://github.com/netlify/open-api/blob/master/CONTRIBUTING.md. To some extent, it might make sense, but a problem arises when you want to make changes particular to the client (e.g. #272). The JS side deals with it by actually encapsulating the swagger.yml into it's npm package - https://github.com/netlify/open-api/blob/master/package.json#L47. This issue won't tackle any of these (this is more to provide some context tbh) as I also believe there are some plans to automatically generate the open-api spec (swagger.yml) from the API side(?).

So when releasing we need to make sure a couple of things happen (not really in a particular order):

  • 1) Bump the version of the package.json
  • 2) Bump the version of the swagger.yml file to match that of the package.json
  • 3) Update the CHANGELOG.md
  • 4) The goclient code is up to date with the swagger.yml, has been generated and pushed

  • release-please takes care of 1) and 3). See the PR - chore: add release-please #288
  • In that same PR I purpose a workaround to deal with 2), where we're essentially updating the swagger.yml file based on the package.json after release-please pushes its changes to a release-* branch.
  • That leaves us with 4):
    • As per @erezrokah suggestion we could setup a verify step on every PR, where we would build and generate the go client and if any changes were detected we would fail in the CI.
    • We would also have the same alternative we've applied for the swagger file version bump, where we could generate the client on the release PR and push it. The downsides being that we would probably be pushing a little to far in terms of what the release phase would be responsible for, the client code in the master branch could potentially be out of sync with the swagger spec and more thorough vetting of the release PR would be required. I would be more inclined to the first option at least initially.

Worth noting that currently the published docs - https://open-api.netlify.com/#/default - result from whatever is in master I believe. There's a potential scenario where master is ahead of what has been release, effectively meaning the version on the swagger.yml might not have been bumped and we would already be presenting "new functionality" as being part of the old version. This could be solved if we only built on tags, not sure if it's something we support?

Finally, another thing to consider is that commits resulting from PR merges should now follow the conventional commit spec - https://www.conventionalcommits.org/en/v1.0.0/ - for release-please to do its thing. As a next step I purpose, we add some CI solution to enforce this 👍

@keiko713
Copy link
Contributor

Thanks for working on this! More streamlined release flow would be wonderful to have and this is great.
I have a question; looks like this will be kicked for every merge to the master, but can we choose not to cut the release for every master update? (it's more of the question for release-please and our policy)

That leaves us with 4)

I'm somewhat in favor of Erez's approach of having verify step on every PR to check. Feels like that's what should happen, if the change to swagger.yml is made with that PR, that PR should be the one who is doing the re-generation of go clients.

Worth noting that currently the published docs - https://open-api.netlify.com/#/default - result from whatever is in master I believe.
This could be solved if we only built on tags, not sure if it's something we support?

Your understanding is correct. I think it's okay to leave it as is for now, but I'd say that this is something we would like to revisit with the open-api updating project. "build only on tags" is something we can tweak in Netlify side, with things like build plugins or maybe we can even do with ignore build command (to ignore the builds unless it's tag? maybe?) I think.

@mraerino
Copy link
Member

looks like this will be kicked for every merge to the master, but can we choose not to cut the release for every master update? (it's more of the question for release-please and our policy)

the release-please workflow only updates a tracking PR with all the changes that have been made since the last release.
only once you merge that tracking PR will a new version be released

@mraerino
Copy link
Member

mraerino commented Jan 20, 2021

Worth noting that currently the published docs - https://open-api.netlify.com/#/default - result from whatever is in master I believe.
This could be solved if we only built on tags, not sure if it's something we support?

Your understanding is correct. I think it's okay to leave it as is for now, but I'd say that this is something we would like to revisit with the open-api updating project. "build only on tags" is something we can tweak in Netlify side, with things like build plugins or maybe we can even do with ignore build command (to ignore the builds unless it's tag? maybe?) I think.

would be cool if we could work with locked deploys and update the published deploy to the right one for the tagged commit once we publish
that way we still get previews for every commit to master

@JGAntunes
Copy link
Contributor

Created #295 to cover commit msg linting so I think we're good to close this one 👍 feel free to re-open it if you feel like something is missing.

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

No branches or pull requests

4 participants