Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements mapping of RUNNER_TEMP environment variable to /github/runner_temp for container actions, controlled by a new feature flag actions_container_action_runner_temp.
Key changes:
- Adds conditional volume mounting and path translation for runner temp directory in container actions
- Introduces new feature flag constant and helper method for controlling the feature
- Provides infrastructure for managing runner temporary directory access within containerized action environments
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Runner.Worker/Handlers/ContainerActionHandler.cs | Adds conditional mounting and path translation for temp directory when feature flag is enabled |
| src/Runner.Worker/FeatureManager.cs | Introduces helper method to check if container action runner temp feature is enabled |
| src/Runner.Common/Constants.cs | Defines the new feature flag constant for container action runner temp |
| ArgUtil.Directory(tempWorkflowDirectory, nameof(tempWorkflowDirectory)); | ||
|
|
||
| container.MountVolumes.Add(new MountVolume("/var/run/docker.sock", "/var/run/docker.sock")); | ||
| if (FeatureManager.IsContainerActionRunnerTempEnabled(ExecutionContext.Global.Variables)) |
There was a problem hiding this comment.
Should we do this also with environment variables? So that we don't have to rely on just feature flags? Same comment for the check below too
Also nit: do we need the helper method since it's a one line helper method?
There was a problem hiding this comment.
The feature flag is temporary for safe rollout only. It will be cleaned up in the following runner release.
I didn't have a reason to add an environment variable check in this case, so I kept it clean and under server control. Environment variables help when customers are involved to help try out a pre-release behavior and confirm it works for their scenarios.
The only reason I added the helper method is to make this file read consistently wrt feature flag checks.
| container.MountVolumes.Add(new MountVolume(tempFileCommandDirectory, "/github/file_commands")); | ||
| container.MountVolumes.Add(new MountVolume(defaultWorkingDirectory, "/github/workspace")); | ||
|
|
||
| if (FeatureManager.IsContainerActionRunnerTempEnabled(ExecutionContext.Global.Variables)) |
There was a problem hiding this comment.
nit : Could we reorder this so that there's just one feature flag check call? The also might help reduce the need for a helper method. Or does the order of operations here matter a lot? E.g. putting the mount of the new temp directory in the feature flag call at the end of the mounting section and then path translate at the beginning of the mapping section?
Sorry quite a picky nit, feel free to ignore this, not sure on whether it makes a difference to the behavior
Edit: also this would then make it easier to do that check plus an env variable check if added, since we'd be able to just check them all once.
Alternatively we can just store the ff value above and then use it.
There was a problem hiding this comment.
I optimized the code, to make the diff easier to read when the feature flag is removed (future PR).
I prefer checking the feature flag twice rather than introduce a local variable. Rule of thumb: more local variables -> worse readability. Checking the feature flag isn't expensive, it's a dictionary lookup.
Maps
RUNNER_TEMPto/github/runner_tempfor container actions.Feature flag:
actions_container_action_runner_tempBelow is a screenshot of the file structure. While testing, I created the file
fooonce inside the container action and confirmed it appeared twice in file structure (touch /github/runner_temp/_runner_file_commands/foo).