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

Run validation before releasing #718

Merged
merged 1 commit into from Mar 24, 2022

Conversation

martyspiewak
Copy link
Contributor

@martyspiewak martyspiewak commented Mar 16, 2022

Changes proposed by this PR

  • Updates release workflow to only create releases off tags from commits on main.
  • Also only creates release if tag is formatted correctly

PR Checklist

Note: Please do not remove items. Mark items as done [x] or use strikethrough if you believe they are not relevant

  • Linked to a relevant issue. Eg: Fixes #123 or Updates #123
  • Removed non-atomic or wip commits
  • Filled in the Release Note section above
  • Modified the docs to match changes

Copy link
Contributor

@idoru idoru left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@netlify
Copy link

netlify bot commented Mar 16, 2022

Deploy Preview for elated-stonebraker-105904 canceled.

Name Link
🔨 Latest commit 92f397b
🔍 Latest deploy log https://app.netlify.com/sites/elated-stonebraker-105904/deploys/623cb3111eebe700084676a0

Copy link
Contributor

@scothis scothis left a comment

Choose a reason for hiding this comment

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

Do we never expect to create a release from a release branch? Or back port a bug fix or security patch to a previous release that isn't on main?

@idoru
Copy link
Contributor

idoru commented Mar 16, 2022

@scothis As of #717 the release is a separately a triggered action and doesn't run tests which now run for every commit on main.

If that's the model going forward, when we have release branches then I imagine we'll also run tests for all commits on those too, and expand this restriction.

@martyspiewak martyspiewak force-pushed the only-create-release-off-main-tags branch from 06c2166 to fa8800d Compare March 18, 2022 13:44
@cirocosta
Copy link
Contributor

cirocosta commented Mar 18, 2022

@idoru so you're saying that we could in validation.yaml remove

name: validation
on:
push:
branches: [main]
pull_request:
branches: [main]

by something like

name: validation
on:
  push:

so we run validation on every commit anywhere, and then drop the check this PR is adding? so that we could, before pushing a tag (from main or a release branch), make sure that the validation did take place, everything went well, and we can release from it?

I think keeping the ability to release from a tag regardless of the branch is very important

@martyspiewak
Copy link
Contributor Author

@cirocosta I'd recommend something more like:

name: validation
on:
  push:
    branches:
      - main
      - release-*

or something to that effect, rather than running validations on every commit to any spike branch we create.

But are you also saying you'd prefer that the check introduced in this PR be removed?

@scothis
Copy link
Contributor

scothis commented Mar 18, 2022

or something to that effect, rather than running validations on every commit to any spike branch we create.

That's what forks are for.

so we run validation on every commit anywhere

It's not every commit, it's every push, just because a commit is on a branch doesn't mean that the workflow will have run for that commit. And certainly doesn't mean that the workflow passed.

If there's one commit I want to makes sure passes all validation it's a tag. Assuming the tagged commit had all the tests pass, I'd still want the release artifacts to undergone the same testing. Two builds of the same source will be very similar, but not identical.

For example, this is a workflow I've found to work well. It builds an artifact once, runs a battery of acceptance tests and then if the run is for a tag, creates a draft release with those artifacts. The released bits are the bits the tests ran against.

@martyspiewak martyspiewak marked this pull request as draft March 23, 2022 13:35
@martyspiewak martyspiewak force-pushed the only-create-release-off-main-tags branch from fa8800d to b119ba4 Compare March 23, 2022 14:27
@martyspiewak martyspiewak marked this pull request as ready for review March 24, 2022 13:27
@martyspiewak martyspiewak force-pushed the only-create-release-off-main-tags branch from 26b57c0 to f4b6210 Compare March 24, 2022 13:50
.github/workflows/release.yaml Outdated Show resolved Hide resolved
* Only release off tags that follow correct syntax
* Pull out shared steps into composite actions
@martyspiewak martyspiewak force-pushed the only-create-release-off-main-tags branch from f4b6210 to 92f397b Compare March 24, 2022 18:06
@martyspiewak martyspiewak changed the title Only create release off tag if commit is on main Run validation before releasing Mar 24, 2022
@martyspiewak martyspiewak requested a review from idoru March 24, 2022 18:06
Copy link
Contributor

@idoru idoru left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@martyspiewak martyspiewak merged commit 2da7804 into main Mar 24, 2022
@martyspiewak martyspiewak deleted the only-create-release-off-main-tags branch March 24, 2022 20:11
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.

None yet

5 participants