-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
helper/ssh: add random number to upload path for script [GH-1545] #1588
Conversation
@@ -25,7 +28,7 @@ const ( | |||
|
|||
// DefaultScriptPath is used as the path to copy the file to | |||
// for remote execution if not provided otherwise. | |||
DefaultScriptPath = "/tmp/script.sh" | |||
DefaultScriptPath = "/tmp/script_%RAND%.sh" |
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.
This is kind of weird but I've decided not to document this since it is a very advanced use case. And it looks like most people don't use this arg anyways. I'd rather not promise any backwards compat on the syntax of this.
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.
Agreed +1
return strings.Replace( | ||
c.ScriptPath, "%RAND%", | ||
strconv.FormatInt(int64(rand.Int31()), 10), -1) | ||
} |
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.
Probably a bit of potential for future confusion that this isn't memoized, but this area of the code is simple enough that I think it's not a big deal.
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.
Maybe, yeah. I think it'll be okay to get started though.
Yep yep this looks good. 👍 |
helper/ssh: add random number to upload path for script [GH-1545]
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Fixes #1545
This makes the default remote path include a random number. There are no backwards incompatibilities here. It was an edge case found by #1545 that if you have multiple
remote-exec
in parallel (possible by targeting other remote hosts from multiple resources), then the scripts would collide.