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.

Sign up for GitHub

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

Jump to bottom

Create ~/.ssh on Windows if it doesn't exist #1745

Merged
merged 1 commit into from
Jun 27, 2022
Merged

Create ~/.ssh on Windows if it doesn't exist #1745

merged 1 commit into from
Jun 27, 2022

Conversation

joshcooper
Copy link
Contributor

@joshcooper joshcooper commented Jun 27, 2022 •
edited
Loading

Beaker executed mkdir -p "~/.ssh" on Windows hosts as part of the prebuilt
steps sequence. However, when using cygwin bash, the double quotes behave like
single quotes, disabling tilde expansion:

$ realpath "~/.ssh"
/home/Administrator/~/.ssh

So '~' was a directory containing the '.ssh' directory. This was never noticed
because all of our VM templates already have the expected ~/.ssh directory. But
if the directory doesn't exist, then later steps would fail trying to write to
~/.ssh/environment.

This commit changes beaker behavior in the case that the path starts with tilde
and does not contain a space, so it's safe to not quote the path.

See commit 54b35b7 which double quoted unix &
windows, which caused a regression that was fixed in 9288623.

@codecov
Copy link

codecov bot commented Jun 27, 2022 •
edited
Loading

Codecov Report

Merging #1745 (582f811) into master (6c0b15b) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1745      +/-   ##
==========================================
+ Coverage   74.88%   74.89%   +0.01%     
==========================================
  Files          82       82              
  Lines        4865     4868       +3     
==========================================
+ Hits         3643     3646       +3     
  Misses       1222     1222
Impacted Files Coverage Δ
lib/beaker/host/windows/exec.rb 55.38% <100.00%> (+2.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c0b15b...582f811. Read the comment docs.

@joshcooper joshcooper added the bug label Jun 27, 2022
This was referenced Jun 27, 2022
Merged
Merged
Merged
@joshcooper
Copy link
Contributor Author

This is an example of how it behaved before. It just happened to work because ~/.ssh already existed:

03:51:09 orphic-fantasy (windows2022-64-1) 10:51:09$ mkdir -p "~/.ssh"
03:51:09 orphic-fantasy (windows2022-64-1) executed in 0.19 seconds
03:51:09 orphic-fantasy (windows2022-64-1) 10:51:09$ chmod 0600 ~/.ssh
03:51:10 orphic-fantasy (windows2022-64-1) executed in 0.22 seconds

This is after:

foggy-induction 12:12:35$ mkdir -p ~/.ssh
foggy-induction executed in 0.19 seconds
foggy-induction 12:12:35$ chmod 0600 ~/.ssh
foggy-induction executed in 0.30 seconds

ekohl
ekohl reviewed Jun 27, 2022
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Thoughts on using Shellwords.escape(str).sub("\\~", "~") instead?

lib/beaker/host/windows/exec.rb Outdated Show resolved Hide resolved
@joshcooper
Copy link
Contributor Author

joshcooper commented Jun 27, 2022 •
edited
Loading

Thoughts on using Shellwords.escape(str).sub("\\~", "~") instead?

I think using Shellwords would be good (as in beaker should have shell escaped things from the beginning), but that's more than I want to take on right now.

@joshcooper
Create ~/.ssh on Windows if it doesn't exist
582f811 
Beaker executed `mkdir -p "~/.ssh"` on Windows hosts as part of the prebuilt
steps sequence. However, when using cygwin bash, the double quotes behave like
single quotes, disabling tilde expansion:

   $ realpath "~/.ssh"
   /home/Administrator/~/.ssh

So '~' was a directory containing the '.ssh' directory. This was never noticed
because all of our VM templates already have the expected ~/.ssh directory. But
if the directory doesn't exist, then later steps would fail trying to write to
~/.ssh/environment.

This commit changes beaker behavior in the case that the path starts with tilde
and does not contain a space, so it's safe to not quote the path.

See commit 54b35b7 which double quoted unix &
windows, which caused a regression that was fixed in 9288623.
@joshcooper
Copy link
Contributor Author

Hi @ekohl I don't want to dismiss your input, but we've got a looming deadline, so I'm going to merge. Happy to chat in the future about improvements to beaker.

@joshcooper joshcooper merged commit a4aa067 into voxpupuli:master Jun 27, 2022
@joshcooper joshcooper deleted the mkdir_p branch June 27, 2022 23:16
@ekohl
Copy link
Member

ekohl commented Jun 28, 2022

No worries. I thought the inline comment was the more important one. The other has a lot of potential gotchas which is why I posed it much more as an open question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl left review comments

@mhashizume mhashizume mhashizume approved these changes

@tvpartytonight tvpartytonight tvpartytonight approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joshcooper @ekohl @tvpartytonight @mhashizume