refactor(agent/agentssh): move envs to agent and add agentssh config struct #12204
Conversation
| @@ -278,8 +278,13 @@ func (r *RootCmd) workspaceAgent() *clibase.Cmd { | |||
| subsystems = append(subsystems, subsystem) | |||
| } | |||
|
|
|||
| procTicker := time.NewTicker(time.Second) | |||
| defer procTicker.Stop() | |||
There was a problem hiding this comment.
Review: This is unused.
| environmentVariables := map[string]string{ | ||
| "GIT_ASKPASS": executablePath, | ||
| } | ||
| if v, ok := os.LookupEnv(agent.EnvProcPrioMgmt); ok { |
There was a problem hiding this comment.
Review: This was dirtying all workspaces with an empty env, even when not set.
| "CODER": "true", // From the agent. | ||
| "MY_MANIFEST": "true", // From the manifest. | ||
| "MY_OVERRIDE": "true", // From the agent environment variables option, overrides manifest. | ||
| "MY_SESSION_MANIFEST": "false", // From the manifest, overrides session env. |
There was a problem hiding this comment.
Review: This is not necessarily correct behavior (IMO), but I wanted to stay truthful to the existing implementation.
| environmentVariables := map[string]string{ | ||
| "GIT_ASKPASS": executablePath, | ||
| } | ||
| if v, ok := os.LookupEnv(agent.EnvProcPrioMgmt); ok { |
There was a problem hiding this comment.
I see that this os.LookupEnv(agent.EnvProcPrioMgmt) appears a few times. I'm wondering if we should cover it with a dedicated function.
There was a problem hiding this comment.
It's only really used here, in the other place we check the map passed to the agent which is better for tests, so I don't think this needs any change at the moment.
There was a problem hiding this comment.
My only concern is the future when somebody decides to introduce another variable and forgets to correct it in the test. Anyway, let's assume it is sort of nitpick 👍
e151c86 to
112a0d0
Compare
b9895ac to
ffae4ad
Compare
ffae4ad to
dd66b45
Compare
This PR refactors where custom environment variables are set in the workspace and decouples
agentspecific configs from theagentssh.Server. To reproduce all functionality,agentssh.Configis introduced.The custom environment variables are now configured in
agent/agent.goand the agent retains control of the final state. This will allow for easier extension in the future and keep other modules decoupled.Related #11131 (follow-up PR).