Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SshCommand: Only force username when requested #63

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

ChlodAlejandro
Copy link
Contributor

This removes the use of the result of whoami in determining the SSH user to run the command as. This means a provided username will be respected, follow by an ~/.ssh/config-defined User, in that order.

This fixes SshCommand ignoring the config-defined User and prevents the use of whoami on Windows, which would return a machine\user format instead of just user (the default when no explicit username is provided when running the built-in OpenSSH client).

This removes the use of the result of `whoami` in determining the SSH
user to run the command as. This means a provided username will be
respected, follow by a config-defined `User`, in that order.

This fixes SshCommand ignoring the config-defined `User` and prevents
the use of `whoami` on Windows, which would return a `machine\user`
format instead of just `user` (the default when no explicit username
is provided when running the built-in OpenSSH client).
@ChlodAlejandro
Copy link
Contributor Author

@MusikAnimal SshCommand was originally introduced in 3b0a99d with the whois step already included. Would removing the extra whois step be okay? I don't see anything in the commit message or Phabricator task that would imply something breaking here, but I wanted to ask out of an abundance of caution.

@MusikAnimal
Copy link
Member

@MusikAnimal SshCommand was originally introduced in 3b0a99d with the whois step already included. Would removing the extra whois step be okay? I don't see anything in the commit message or Phabricator task that would imply something breaking here, but I wanted to ask out of an abundance of caution.

@ChlodAlejandro To be honest, I don't remember adding the whoami code seen on L48-50, lol. If it needs to be rewritten or something, go for it, so long as the command works without Docker too and without specifying a username (we want to make connecting as simple as possible).

@ChlodAlejandro
Copy link
Contributor Author

ChlodAlejandro commented Jul 25, 2023

This should be fine as-is then. Thanks! PR is ready for reviews.

Copy link
Member

@MusikAnimal MusikAnimal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great in my testing! Merging

@MusikAnimal MusikAnimal merged commit ce284d3 into wikimedia:master Jul 25, 2023
4 checks passed
@samwilson
Copy link
Member

Release tagged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants