-
Notifications
You must be signed in to change notification settings - Fork 259
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
WCOW: Use commandline in spec if populated #467
Conversation
|
||
// escapeArgs makes a Windows-style escaped command line from a set of arguments | ||
func escapeArgs(args []string) string { | ||
escapedArgs := make([]string, len(args)) |
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.
You could use a strings.Builder
and avoid the additional array alloc
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.
@jterry75 Happy to change if you think it's important enough.
Seems fine to me. Let me know when this gets upstreamed and we get this officially |
f1d25b6
to
a79ddd5
Compare
Update LGTM. I like that we no longer accept |
a79ddd5
to
9a813f6
Compare
Signed-off-by: John Howard <jhoward@microsoft.com>
9a813f6
to
a9d902e
Compare
Signed-off-by: John Howard <jhoward@microsoft.com> This is the version with `CommandLine` added.
a9d902e
to
31fdae3
Compare
@jterry75 This one is ready to go. Updated to the latest vendor of the runtime-spec. |
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
Signed-off-by: John Howard jhoward@microsoft.com
@jterry75 Don't merge yet - the vendor part is hacked pending opencontainers/runtime-spec#998 being merged. Have done end-to-end verification of this working with docker with the containerd back-end for runtime.