-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Tiny tweaks and fixes for the PowerShell provisioner #11410
Conversation
if os.IsPathSeparator(p.config.RemotePath[len(p.config.RemotePath)-1]) { | ||
// path is a directory | ||
p.config.RemotePath += filepath.Base((fi).Name()) | ||
p.config.RemotePath += filepath.Base(fi.Name()) |
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.
os.IsPathSeparator
on Windows will check for \
and /
! filepath.Base
internally uses IsPathSeparator
to find the base path, so it should be fine.
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.
Nice 👍 - thanks for the call out here.
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.
Overall this looks like a much needed change. There are two small updates needed for the documentation to wrap up the change. See comments inline.
Approving in advance to unblock the merge.
if os.IsPathSeparator(p.config.RemotePath[len(p.config.RemotePath)-1]) { | ||
// path is a directory | ||
p.config.RemotePath += filepath.Base((fi).Name()) | ||
p.config.RemotePath += filepath.Base(fi.Name()) |
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.
Nice 👍 - thanks for the call out here.
~> On Windows, if possible, try to always use a backslash `\` as a path | ||
separator, especially when dealing with non full paths. This is because when | ||
|
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.
The examples below use /
and not \
should this suggestion be updated?
I think this rationale will be helpful "This is because when..."
~> On Windows, if possible, try to always use a backslash `\` as a path | |
separator, especially when dealing with non full paths. This is because when | |
~> On Windows, if possible, try to always use a backslash `\` as the path | |
separator, especially when dealing with relative paths. The use of backslashes helps with [....] | |
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.
Ha yes, good catch, this is a WIP commit I pushed before we switched over to the acc tests branch ! Fixing now.
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 made it clearer; and now wonder if we should display this in other places too. Ah, I will make a template out of this !
make it a note
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
fix #6188