-
Notifications
You must be signed in to change notification settings - Fork 329
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
Add resolve-environment
action
#1684
Conversation
1dca7fb
to
bbaaef5
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.
This is looking in good shape!
- I think it's worth writing a CHANGELOG note for observant customers who are watching the repo to say that this Action is an internal experiment, is subject to change and shouldn't be used in production.
- It'd be good to have a basic integration test of this once we have a CLI to test it against, just to make sure everything is wired up correctly.
- We will need to make some internal changes to start capturing telemetry properly — for instance we need to add
resolve-environment
to an allowlist in the monolith. Though there's no rush to make this change.
a01d376
to
f95520d
Compare
6a72d37
to
f95520d
Compare
); | ||
core.setOutput("environment", result); | ||
} catch (unwrappedError) { | ||
const error = wrapError(unwrappedError); |
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.
Are there any known user errors that we should ignore? Eg- trying to resolve an unknown language? Trying to avoid spurious triggers of our SLOs.
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.
I think this will probably get addressed by the proposed handling of CLI errors in general: #1684 (comment)
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.
Nice! Some minor suggestions to improve this before we merge.
fa9ebea
to
0485950
Compare
Co-authored-by: Henry Mercer <[email protected]>
Co-authored-by: Henry Mercer <[email protected]>
0485950
to
c878505
Compare
In addition to the basic integration test included in this PR, we also have an internal repo with a more comprehensive test matrix which covers most cases that the Go autobuilder distinguishes between when resolving build environments. That forms an end-to-end test for all the new components related to this work. |
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.
One improvement around better CLI error handling for old CLIs, otherwise this LGTM.
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.
Checks are currently failing since nightly-latest
doesn't have a version number of 2.13.4+ yet. I suggest we point to the new 2.13.4 bundle directly (stable-v2.13.4
) or alternatively wait until #1721 is merged and use latest
in the checks.
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.
Thanks for addressing the feedback on the naming of the check step! This LGTM
Summary
This PR adds a new
resolve-environment
action which calls the new CLIresolve build-environment
command to try and infer a configuration for the build environment that is suitable for the autobuilder. Example workflow (assuming a repository with a Go project):The action performs the following work:
tools
input); same behaviour as theinit
action.resolve build-environment
command and stores the JSON result received from that in theenvironment
output