- Notifications
You must be signed in to change notification settings - Fork928
refactor(agent/agentssh): move envs to agent and add agentssh config struct#12204
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Review: This is not necessarily correct behavior (IMO), but I wanted to stay truthful to the existing implementation.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| environmentVariables := map[string]string{ | ||
| "GIT_ASKPASS": executablePath, | ||
| } | ||
| if v, ok := os.LookupEnv(agent.EnvProcPrioMgmt); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I see that thisos.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 👍
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM once CI is happy!
e151c86 to112a0d0Compareb9895ac toffae4adCompareffae4ad todd66b45Compare
Uh oh!
There was an error while loading.Please reload this page.
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).