Skip to content

Adds build job to needs list for safer deploy #1235

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.

Sign up for GitHub

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

Open
zweatshirt wants to merge 1 commit into
base: master
Choose a base branch
from
+1 −1
Open

Adds build job to needs list for safer deploy #1235

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ jobs:
runs-on: ubuntu-latest
environment:
name: ${{ (github.ref == 'refs/heads/master' && 'production') || 'staging' }}
needs: [lint, test]
needs: [lint, test, build]
Copy link
Contributor

@canac canac Aug 28, 2025

Choose a reason for hiding this comment

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

What scenario does this prevent against? If the build job would fail, wouldn't the yarn run build step in this deployment job fail also and abort the deployment anyway?

Copy link
Contributor Author

@zweatshirt zweatshirt Aug 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That's true. It was my thought that that increases the time it takes for the workflow to process. If build fails, it didn't seem practical to re-run build in the deployment job despite failure. Maybe I am misunderstanding it. I guess it doesn't matter, since build will be flagged as fail anyway.

Copy link
Contributor

@canac canac Aug 28, 2025

Choose a reason for hiding this comment

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

That's a good point that it avoids rebuilding if the first build fails. Thanks for thinking of that improvement!

I'm thinking that this will make deployments slower though when the build succeeds. Now it will have to build, wait, then rebuild and deploy. Before the two builds could run in parallel and now they must run sequentially. I lean towards optimizing for the speed of deploying successful builds. Although in practice, building usually takes only ~15 seconds longer than running the tests, so this change wouldn't slow things down too much.

Copy link
Contributor

@dr-bizz dr-bizz Aug 28, 2025

Choose a reason for hiding this comment

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

In practice, I prefer that we have a successful build first before trying to deploy. I know this is happening anyway, as the deploy job runs a build.

I'm not fussed; I can go either way.

I will say our normal process is to build, and then once it's successful, we deploy. MPDX works this way.

Since we build on the CI when asking Amplify to deploy. Then Amplify runs a build to deploy.

if: (github.event_name == 'push' && (github.ref == 'refs/heads/master' || github.ref == 'refs/heads/staging')) || (github.event_name == 'workflow_dispatch' && github.event.inputs.auto-deploy == 'true')
steps:
- uses: actions/checkout@v2
Expand Down