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

Add resolve-environment action #1684

Merged
merged 33 commits into from
Jun 15, 2023
Merged

Add resolve-environment action #1684

merged 33 commits into from
Jun 15, 2023

Conversation

mbg
Copy link
Member

@mbg mbg commented May 18, 2023

Summary

This PR adds a new resolve-environment action which calls the new CLI resolve 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):

jobs:
  analyze-linux:
    runs-on: ubuntu-latest

    steps:
      - uses: actions/checkout@v3

      - uses: github/codeql-action/resolve-environment@mbg/add-resolve-environment
        id: resolve-environment
        with:
          language: go

      - run: echo "${{ steps.resolve-environment.outputs.environment }}"

The action performs the following work:

  • Downloads the CodeQL CLI (optionally from the tools input); same behaviour as the init action.
  • Runs the resolve build-environment command and stores the JSON result received from that in the environment output

@mbg mbg self-assigned this May 18, 2023
@mbg mbg force-pushed the mbg/add-resolve-environment branch from 1dca7fb to bbaaef5 Compare May 26, 2023 10:40
Copy link
Contributor

@henrymercer henrymercer left a 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.

resolve-environment/action.yml Outdated Show resolved Hide resolved
resolve-environment/action.yml Outdated Show resolved Hide resolved
src/resolve-environment-action.ts Outdated Show resolved Hide resolved
src/resolve-environment-action.ts Outdated Show resolved Hide resolved
src/resolve-environment.ts Outdated Show resolved Hide resolved
src/codeql.ts Outdated Show resolved Hide resolved
src/resolve-environment-action.ts Outdated Show resolved Hide resolved
@mbg mbg force-pushed the mbg/add-resolve-environment branch 3 times, most recently from a01d376 to f95520d Compare June 6, 2023 11:22
@mbg mbg force-pushed the mbg/add-resolve-environment branch from 6a72d37 to f95520d Compare June 13, 2023 11:06
@mbg mbg marked this pull request as ready for review June 13, 2023 11:11
@mbg mbg requested a review from a team as a code owner June 13, 2023 11:11
CHANGELOG.md Outdated Show resolved Hide resolved
);
core.setOutput("environment", result);
} catch (unwrappedError) {
const error = wrapError(unwrappedError);
Copy link
Contributor

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.

Copy link
Member Author

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)

src/resolve-environment-action.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@henrymercer henrymercer left a 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.

CHANGELOG.md Outdated Show resolved Hide resolved
pr-checks/checks/resolve-environment-action.yml Outdated Show resolved Hide resolved
resolve-environment/action.yml Outdated Show resolved Hide resolved
resolve-environment/action.yml Outdated Show resolved Hide resolved
src/codeql.ts Outdated Show resolved Hide resolved
src/resolve-environment-action.ts Outdated Show resolved Hide resolved
src/resolve-environment-action.ts Outdated Show resolved Hide resolved
src/resolve-environment-action.ts Outdated Show resolved Hide resolved
src/resolve-environment.ts Outdated Show resolved Hide resolved
src/resolve-environment.ts Outdated Show resolved Hide resolved
@mbg mbg force-pushed the mbg/add-resolve-environment branch from fa9ebea to 0485950 Compare June 13, 2023 19:15
@mbg mbg force-pushed the mbg/add-resolve-environment branch from 0485950 to c878505 Compare June 13, 2023 19:47
@mbg
Copy link
Member Author

mbg commented Jun 13, 2023

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.

@mbg mbg requested review from henrymercer and aeisenberg June 14, 2023 15:04
Copy link
Contributor

@henrymercer henrymercer left a 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.

src/resolve-environment-action.ts Show resolved Hide resolved
@mbg mbg requested a review from henrymercer June 15, 2023 09:15
henrymercer
henrymercer previously approved these changes Jun 15, 2023
Copy link
Contributor

@henrymercer henrymercer left a 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.

Copy link
Contributor

@henrymercer henrymercer left a 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 :shipit:

@mbg mbg merged commit 0ac1815 into main Jun 15, 2023
@mbg mbg deleted the mbg/add-resolve-environment branch June 15, 2023 16:50
@github-actions github-actions bot mentioned this pull request Jun 21, 2023
6 tasks
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.

3 participants