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

Set runner environment in context and env #2518

Merged
merged 2 commits into from
May 26, 2023

Conversation

feelepxyz
Copy link
Contributor

@feelepxyz feelepxyz commented Apr 4, 2023

Problem

runner_environment was added to the OIDC id token claims but it's currently not exposed in the runner context or ENV.

We would like to access the runner env value from the system ENV on the runner in order to include it in the provenance statement generated in the npm CLI when constructing the builder.id: https://github.com/npm/cli/blob/latest/workspaces/libnpmpublish/lib/provenance.js#L7

@feelepxyz feelepxyz changed the title WIP: Set runner environment in context and env Set runner environment in context and env May 23, 2023
@feelepxyz feelepxyz force-pushed the feelepxyz/add-runner-env branch 3 times, most recently from ed1209c to 8a1090d Compare May 23, 2023 16:15
@feelepxyz feelepxyz marked this pull request as ready for review May 23, 2023 16:15
@feelepxyz feelepxyz requested a review from a team as a code owner May 23, 2023 16:15
Extract runner_environment from the global context and expose in the
`github.runner` context and env as `RUNNER_ENVIRONMENT`.

Signed-off-by: Philip Harrison <philip@mailharrison.com>
@@ -126,6 +126,11 @@ public async Task<TaskResult> RunAsync(AgentJobRequestMessage message, Cancellat
_runnerSettings = HostContext.GetService<IConfigurationStore>().GetSettings();
jobContext.SetRunnerContext("name", _runnerSettings.AgentName);

if (jobContext.Global.Variables.TryGetValue(WellKnownDistributedTaskVariables.RunnerEnvironment, out var runnerEnvironment))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TingluoHuang presuming this tryGetValue should make it safe to deploy this change independently of the backend change?

Also had a poke around and doesn't look like there's any allow list for what goes in the RUNNER_{KEY} ENV values: https://github.com/actions/runner/blob/main/src/Runner.Worker/RunnerContext.cs

Looks like the GitHub context does have an allow list: https://github.com/actions/runner/blob/main/src/Runner.Worker/GitHubContext.cs#L9

Copy link
Member

Choose a reason for hiding this comment

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

We don't have an allow list before for the RUNNER_* since before this change all RUNNER_* are set by the runner, while most GITHUB_* are coming from the service.

@@ -126,6 +126,11 @@ public async Task<TaskResult> RunAsync(AgentJobRequestMessage message, Cancellat
_runnerSettings = HostContext.GetService<IConfigurationStore>().GetSettings();
jobContext.SetRunnerContext("name", _runnerSettings.AgentName);

if (jobContext.Global.Variables.TryGetValue(WellKnownDistributedTaskVariables.RunnerEnvironment, out var runnerEnvironment))
{
jobContext.SetRunnerContext("environment", runnerEnvironment);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TingluoHuang another q: Looking at these docs: https://docs.github.com/en/actions/learn-github-actions/variables#detecting-the-operating-system - it seems you can access the runner context before the job runs, e.g. if: runner.os == 'Windows'. From our sync yesterday, is this a problem for us? Presuming it should be ok to set it here as everything else is set here too.

Copy link
Member

Choose a reason for hiding this comment

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

if: runner.os == 'Windows' in the example in under steps (not before job land on the runner), which will be on the runner for sure.

TingluoHuang
TingluoHuang previously approved these changes May 26, 2023
@TingluoHuang TingluoHuang merged commit 21b49c5 into actions:main May 26, 2023
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.

2 participants