Conversation
Add env vars for top-level primitives in job context job.check_run_id => JOB_CHECK_RUN_ID job.status => JOB_STATUS
There was a problem hiding this comment.
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
IEnvironmentContextDatainterface onJobContextclass - 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); |
There was a problem hiding this comment.
Implicit conversion from StringContextData to string may not work as expected. Use value.ToString() to explicitly convert the value to a string.
| yield return new KeyValuePair<string, string>($"JOB_{data.Key.ToUpperInvariant()}", value); | |
| yield return new KeyValuePair<string, string>($"JOB_{data.Key.ToUpperInvariant()}", value.ToString()); |
| { | ||
| if (data.Value is StringContextData value) | ||
| { | ||
| yield return new KeyValuePair<string, string>($"JOB_{data.Key.ToUpperInvariant()}", value); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_andrunner.->RUNNER_ - There's potentially a risk of collisions if any
github.job_...fields are added in the future, since those would also have aGITHUB_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
There was a problem hiding this comment.
@jenseng I think you are right your name pattern makes sense then
|
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🤞 |
|
@TingluoHuang @ericsciple @pje I've noticed you've all created or reviewed PRs related to this file. Is this something you'd be able to take a look at and help merge? It would help folks use this new excellent flag in a way that is conventional with how they are checking other similar properties (using environment variables). Would be a huge boon for our ability to deep link into jobs and the change looks smallish so would love it if you could take a look and help get this merged if possible 🙏🏼 |
|
? |
3 participants
Add env vars for top-level job context primitives, i.e.
job.check_run_id=>$JOB_CHECK_RUN_IDjob.status=>$JOB_STATUSReferences