- Notifications
You must be signed in to change notification settings - Fork924
chore(agent/agentssh): extract EnvInfoer#16603
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
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.
Naming aside, changes look good so approving 👍🏻
| // UserHomeDir returns the home directory of the current user. | ||
| UserHomeDir() (string, error) | ||
| // UserShell returns the shell of the given user. | ||
| UserShell(username string) (string, error) |
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 feel like all these methods are related to the environment, figuring out the user, env, home and shell, so EnvInfo sounds like a pretty good name to me. Deps is a bit vague IMO and makes me think of dependency injection. 😄
We could also simplify this interface somewhat. All we really need are:
User()Environ()
We'd just make sure the returned user has the correct home directory and shell populated. Not sure if that'd be worth the lift, though.
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.
EnvInfoer it is!
I'd prefer to do the bare minimum here, but we can consolidate + simplify down the line!
| @@ -96,7 +96,7 @@ func TestNewServer_ExecuteShebang(t *testing.T) { | |||
| t.Run("Args", func(t *testing.T) { | |||
| t.Parallel() | |||
| cmd, err := s.CreateCommand(ctx, `#!/usr/bin/env bash | |||
| echo test`, nil) | |||
| echo test`, nil, nil) | |||
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.
Perhaps we should add a test for a custom implementation as well?
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.
Can do!
4edd77b intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Extracts environment-level dependencies of
agentssh.Server.CreateCommand()to an interface to allow alternative implementations to be passed in.We'll need this so we can pass in an alternative implementation that can reference a running container instead of the agent's environment.
I'm not married to the naming convention and will rename the thing if anyone has a better name.