-
Notifications
You must be signed in to change notification settings - Fork 607
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
Use printf instead of $'' to pass a multi-line string to bash #2874
Conversation
b2cfd6a
to
4fd3db4
Compare
Seems like |
Can we move this to v1.0.2? |
Sure. It is a regression to pre-1.0.0 releases, but only affects users that have switched their default shell inside the instance to |
3a4d151
to
10b5f51
Compare
Just a small quoting issue; should be all good now. |
10b5f51
to
334a8df
Compare
The script will be executed by the default shell of the ssh user, and `fish` does not support the $'' syntax. Signed-off-by: Jan Dubois <[email protected]>
334a8df
to
b4252cc
Compare
// double up all '%' characters so we can pass them through unchanged in the format string of printf | ||
interpreter = strings.ReplaceAll(interpreter, "%", "%%") | ||
exportParam = strings.ReplaceAll(exportParam, "%", "%%") | ||
// strings will be interpolated into single-quoted strings, so protect any existing single quotes | ||
interpreter = strings.ReplaceAll(interpreter, "'", `'"'"'`) | ||
exportParam = strings.ReplaceAll(exportParam, "'", `'"'"'`) |
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.
All these replacements are not really needed because the input strings don't include %
or '
characters. but it feels like the right thing to do to make them correct for that case 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.
LGTM 👍🏻
However, I feel that ssh.ExecuteScript
should use shellwords.Parse
to split the first line into a command and arguments rather than using the entire line as the command itself.
🙏
Given that |
The script will be executed by the default shell of the ssh user, and
fish
does not support the $'' syntax.Fixes #2856
Seems to work: