Skip to content

Conversation

jenseng
Copy link

@jenseng jenseng commented Sep 22, 2025

Add env vars for top-level job context primitives, i.e.

job.check_run_id => $JOB_CHECK_RUN_ID
job.status => $JOB_STATUS

References

Add env vars for top-level primitives in job context

  job.check_run_id => JOB_CHECK_RUN_ID
  job.status => JOB_STATUS
@jenseng jenseng requested a review from a team as a code owner September 22, 2025 20:37
@Copilot Copilot AI review requested due to automatic review settings September 22, 2025 20:37
Copy link
Contributor

@Copilot Copilot AI left a 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 adds support for exposing job context primitives as environment variables by implementing the IEnvironmentContextData interface on JobContext. The implementation allows job.check_run_id and job.status to be accessible as $JOB_CHECK_RUN_ID and $JOB_STATUS environment variables respectively.

  • Implements IEnvironmentContextData interface on JobContext class
  • Adds GetRuntimeEnvironmentVariables() method to expose job context data as environment variables
  • Includes comprehensive unit tests to verify the environment variable generation functionality

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/Runner.Worker/JobContext.cs Implements IEnvironmentContextData interface and adds method to convert job context properties to environment variables
src/Test/L0/Worker/JobContextL0.cs Adds unit tests to verify correct environment variable generation from job context data

{
if (data.Value is StringContextData value)
{
yield return new KeyValuePair<string, string>($"JOB_{data.Key.ToUpperInvariant()}", value);
Copy link

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

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

Implicit conversion from StringContextData to string may not work as expected. Use value.ToString() to explicitly convert the value to a string.

Suggested change
yield return new KeyValuePair<string, string>($"JOB_{data.Key.ToUpperInvariant()}", value);
yield return new KeyValuePair<string, string>($"JOB_{data.Key.ToUpperInvariant()}", value.ToString());

Copilot uses AI. Check for mistakes.

{
if (data.Value is StringContextData value)
{
yield return new KeyValuePair<string, string>($"JOB_{data.Key.ToUpperInvariant()}", value);
Copy link

Choose a reason for hiding this comment

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

I would go for a GITHUB_JOB_ prefix instead of just JOB_ to make it more consistent to other environment variables and to point out that these variables are github context variables.

Copy link
Author

Choose a reason for hiding this comment

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

That's not a bad idea, I'm curious to see what the maintainers think. I had originally considered that, but:

  • It makes it less consistent with how other context -> env vars work, i.e. github. -> GITHUB_ and runner. -> RUNNER_
  • There's potentially a risk of collisions if any github.job_... fields are added in the future, since those would also have a GITHUB_JOB_ prefix

Personally my preference would have been to name the context field github.job_id, in which case the env var could just be GITHUB_JOB_ID, but I think that ship has sailed 🫤

cc @ericsciple

Copy link

Choose a reason for hiding this comment

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

@jenseng I think you are right your name pattern makes sense then

kzu added a commit to devlooped/dotnet-trx that referenced this pull request Sep 29, 2025
kzu added a commit to devlooped/dotnet-trx that referenced this pull request Sep 29, 2025
kzu added a commit to devlooped/dotnet-trx that referenced this pull request Sep 29, 2025
@alexevanczuk
Copy link

Hey @jenseng wondering if you're still hoping to get this merged 🙏🏼 excited about it!

@jenseng
Copy link
Author

jenseng commented Oct 13, 2025

Hey @jenseng wondering if you're still hoping to get this merged 🙏🏼 excited about it!

@alexevanczuk absolutely! Just waiting for the GitHub folks to review it🤞

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