-
Notifications
You must be signed in to change notification settings - Fork 366
Fail build.sh
if any command in it fails
#2931
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
base: main
Are you sure you want to change the base?
Conversation
71d8dee
to
63beaad
Compare
63beaad
to
8f71d47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR ensures that Go is explicitly installed in all local workflow runs by adding an actions/setup-go@v5
step, so that build.sh
and related scripts will correctly fail if Go is missing.
- Introduces an Install Go step to every workflow to guarantee a supported Go version is available.
- Aligns all workflows on
go-version: '>=1.21.0'
and disables caching of Go installs. - Prepares the ground for
build.sh
to error out early when dependencies are not met.
Reviewed Changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
.github/workflows/__test-local-codeql.yml | Added Install Go step before fetching bundle |
.github/workflows/__swift-custom-build.yml | Added Install Go step before init action |
.github/workflows/__split-workflow.yml | Added Install Go step before init action |
.github/workflows/__remote-config.yml | Added Install Go step before init action |
.github/workflows/__packaging-inputs-js.yml | Added Install Go step before init action |
.github/workflows/__packaging-config-js.yml | Added Install Go step before init action |
.github/workflows/__packaging-config-inputs-js.yml | Added Install Go step before init action |
.github/workflows/__packaging-codescanning-config-inputs-js.yml | Added Install Go step before init action |
.github/workflows/__multi-language-autodetect.yml | Added Install Go step before init action |
.github/workflows/__go-tracing-legacy-workflow.yml | Replaced old setup-go usage with named step |
.github/workflows/__go-tracing-custom-build-steps.yml | Replaced old setup-go usage with named step |
.github/workflows/__go-tracing-autobuilder.yml | Replaced old setup-go usage with named step |
.github/workflows/__go-indirect-tracing-workaround.yml | Added Install Go step before init action |
.github/workflows/__go-indirect-tracing-workaround-no-file-program.yml | Added Install Go step before remove step |
.github/workflows/__go-indirect-tracing-workaround-diagnostic.yml | Added Install Go step before init action |
.github/workflows/__go-custom-queries.yml | Added Install Go step before init action |
.github/workflows/__export-file-baseline-information.yml | Added Install Go step before init action |
.github/workflows/__build-mode-manual.yml | Added Install Go step before init action |
.github/workflows/__analyze-ref-input.yml | Added Install Go step before init action |
.github/workflows/__all-platform-bundle.yml | Added Install Go step before init action |
Comments suppressed due to low confidence (1)
.github/workflows/__test-local-codeql.yml:48
- This
Install Go
step is duplicated across many workflows. Consider extracting it into a reusable workflow snippet or composite action to simplify updates and reduce duplication.
- - name: Install Go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nits only on my side.
installGo = False | ||
if checkSpecification.get('installGo'): | ||
installGo = True if checkSpecification['installGo'].lower() == "true" else False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we just use
installGo = False | |
if checkSpecification.get('installGo'): | |
installGo = True if checkSpecification['installGo'].lower() == "true" else False | |
installGo = checkSpecification.get('installGo') |
then we can use a more natural
installGo: true
in the test specification, without using stringly typed values. Although I see we use "true"
for other cases as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, like with the other point, I went with this to mirror the existing inputs. I would be happy to change this though if we can't think of a downside / motivation for why the others are strings.
This PR updates
build.sh
so that it fails if any step in the script fails. Previously, the script would continue running and thus mask unexpected failures. For example, Go is no longer available by default on runners and so an explicitsetup-go
step was required. However, because the script didn't fail, we didn't notice that failure until now.To address the Go issue, I updated
sync.sh
to understand a newinstallGo
input that injects thesetup-go
step into the workflow.I tried adding it to the local
prepare-test
action first, but this resulted in a number of further issues:setup-go
there, which has a post-action, forced the entireprepare-test
action to have a post-action, which caused problems because the workflow also moves the location of theprepare-test
action to outside of the workspace..github
folder in the workspace.Merge / deployment checklist