GitHub rsync escaping #15589

coreydaley · 2017-08-01T15:14:43Z

Correctly escapes oc rsync command arguments

bparees · 2017-08-01T19:18:50Z

(fyi corey was testing something for me, not trying to ram his own PRs through :) )

bparees · 2017-08-01T19:24:55Z

/retest

coreydaley · 2017-08-02T17:24:49Z

@bparees @jim-minter added unit test

bparees · 2017-08-02T17:48:06Z

[lgtm removed because the bot is being weird]

will let @jim-minter have the last word though.

stevekuznetsov · 2017-08-02T18:51:59Z

@bparees you may want to /lgtm cancel

bparees · 2017-08-02T19:05:16Z

@stevekuznetsov removing my comment and deleting the labels manually seems to have been sufficient to reign in the bot.

jim-minter · 2017-08-03T17:05:24Z

pkg/oc/cli/cmd/rsync/copyrsync_test.go

+
+// TestRsyncEscapeCommand ensures that command line options supplied to oc rsync
+// are properly escaped.
+func TestRsyncEscapeCommand(t *testing.T) {


nit: using raw strings (quote with `) will make this easier to validate

jim-minter · 2017-08-03T17:05:43Z

pkg/oc/cli/cmd/rsync/copyrsync.go

+// Example: " -> ""
+func rsyncEscapeCommand(command []string) []string {
+	for key := range command {
+		needsQuoted := strings.ContainsAny(command[key], `'" \`)


don't believe the backslash is necessary in this line

jim-minter · 2017-08-03T17:06:34Z

pkg/oc/cli/cmd/rsync/copyrsync.go

+// if it contains any of the following: single quote, double quote, backslash, or space
+// It also replaces every pre-existing double quote character in the element with a pair.
+// Example: " -> ""
+func rsyncEscapeCommand(command []string) []string {


note that this function modifies its argument in place and returns it, which is a little unusual

coreydaley · 2017-08-03T18:03:32Z

@jim-minter comments addressed

coreydaley · 2017-08-03T19:34:31Z

/retest

stevekuznetsov · 2017-08-04T00:05:30Z

/test extended_conformance_install_update

stevekuznetsov · 2017-08-04T01:45:28Z

/test extended_conformance_install_update

stevekuznetsov · 2017-08-04T02:14:30Z

/test extended_conformance_install_update

coreydaley · 2017-08-04T11:58:36Z

@bparees @jim-minter ptal
comments have been addressed and tests are green

0xmichalis · 2017-08-04T12:47:40Z

/test extended_templates

jim-minter · 2017-08-04T19:19:12Z

/lgtm

0xmichalis · 2017-08-04T19:28:23Z

/retest

bparees · 2017-08-04T20:13:14Z

/approve Ben Parees | OpenShift

…

On Aug 4, 2017 3:19 PM, "OpenShift Merge Robot" ***@***.***> wrote: [APPROVALNOTIFIER] This PR is *NOT APPROVED* This pull-request has been approved by: *coreydaley <#15589#>*, *jim-minter <#15589 (comment)>* We suggest the following additional approver: *bparees* Assign the PR to them by writing /assign @bparees in a comment when ready. The full list of commands accepted by this bot can be found here <https://github.com/kubernetes/test-infra/blob/master/commands.md>. Needs approval from an approver in each of these OWNERS Files: - *pkg/cmd/OWNERS <https://github.com/openshift/origin/blob/master/pkg/cmd/OWNERS>* - *pkg/oc/OWNERS <https://github.com/openshift/origin/blob/master/pkg/oc/OWNERS>* You can indicate your approval by writing /approve in a comment You can cancel your approval by writing /approve cancel in a comment — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15589 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEvl3pk9fWw9zUSrUFvVL-s6EU8biCdgks5sU27DgaJpZM4Op4SD> .

openshift-merge-robot · 2017-08-04T20:13:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, coreydaley, jim-minter

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cmd/OWNERS~~ [bparees]
~~pkg/oc/OWNERS~~ [bparees]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-08-04T23:03:45Z

/test all [submit-queue is verifying that this PR is safe to merge]

coreydaley · 2017-08-05T00:36:12Z

/retest

…

On Aug 4, 2017 7:16 PM, "OpenShift CI Robot" ***@***.***> wrote: @coreydaley <https://github.com/coreydaley>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command ci/openshift-jenkins/extended_conformance_install_update 77e5ea7 <77e5ea7> link <https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended_conformance_install_update/3552> /test extended_conformance_install_update Full PR test history <https://openshift-gce-devel.appspot.com/pr/15589>. Your PR dashboard <https://openshift-gce-devel.appspot.com/pr/coreydaley>. Please help us cut down on flakes by linking to <https://github.com/kubernetes/community/blob/master/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/openshift/origin/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://github.com/kubernetes/test-infra/blob/master/commands.md>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15589 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE02zbHZeXhEX5yen4tps94p5zUacXSMks5sU6Y6gaJpZM4Op4SD> .

openshift-merge-robot · 2017-08-05T05:26:52Z

/test all [submit-queue is verifying that this PR is safe to merge]

coreydaley · 2017-08-06T19:00:01Z

/retest

…

On Aug 5, 2017 3:36 AM, "OpenShift CI Robot" ***@***.***> wrote: @coreydaley <https://github.com/coreydaley>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command ci/openshift-jenkins/extended_conformance_gce 77e5ea7 <77e5ea7> link <https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended_conformance_gce/5421> /test extended_conformance_gce Full PR test history <https://openshift-gce-devel.appspot.com/pr/15589>. Your PR dashboard <https://openshift-gce-devel.appspot.com/pr/coreydaley>. Please help us cut down on flakes by linking to <https://github.com/kubernetes/community/blob/master/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/openshift/origin/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://github.com/kubernetes/test-infra/blob/master/commands.md>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15589 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE02zRghlLVgQeabDzgEBPoZLysQYidiks5sVBtwgaJpZM4Op4SD> .

openshift-merge-robot · 2017-08-06T21:34:43Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-bot · 2017-08-07T00:27:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2017-08-07T08:13:07Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2017-08-07T10:27:17Z

Automatic merge from submit-queue

openshift-merge-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 1, 2017

openshift-merge-robot assigned bparees and fabianofranz Aug 1, 2017

bparees assigned jim-minter and unassigned fabianofranz Aug 1, 2017

coreydaley added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2017

bparees removed the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2017

coreydaley added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 1, 2017

openshift-merge-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 2, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2017

bparees removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2017

bparees removed approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Aug 2, 2017

openshift deleted a comment from openshift-ci-robot Aug 2, 2017

jim-minter reviewed Aug 3, 2017

View reviewed changes

Properly escaping the arguments supplied to rsync -e

77e5ea7

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 4, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2017

openshift-merge-robot merged commit ef91f06 into openshift:master Aug 7, 2017

GitHub rsync escaping #15589

GitHub rsync escaping #15589

Conversation

coreydaley commented Aug 1, 2017

Uh oh!

bparees commented Aug 1, 2017

Uh oh!

bparees commented Aug 1, 2017

Uh oh!

coreydaley commented Aug 2, 2017

Uh oh!

bparees commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevekuznetsov commented Aug 2, 2017

Uh oh!

bparees commented Aug 2, 2017

Uh oh!

jim-minter Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

jim-minter Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

jim-minter Aug 3, 2017

Choose a reason for hiding this comment

Uh oh!

coreydaley commented Aug 3, 2017

Uh oh!

coreydaley commented Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevekuznetsov commented Aug 4, 2017

Uh oh!

stevekuznetsov commented Aug 4, 2017

Uh oh!

stevekuznetsov commented Aug 4, 2017

Uh oh!

coreydaley commented Aug 4, 2017

Uh oh!

0xmichalis commented Aug 4, 2017

Uh oh!

jim-minter commented Aug 4, 2017

Uh oh!

0xmichalis commented Aug 4, 2017

Uh oh!

bparees commented Aug 4, 2017 via email

Uh oh!

openshift-merge-robot commented Aug 4, 2017

Uh oh!

openshift-merge-robot commented Aug 4, 2017

Uh oh!

coreydaley commented Aug 5, 2017 via email

Uh oh!

openshift-merge-robot commented Aug 5, 2017

Uh oh!

coreydaley commented Aug 6, 2017 via email

Uh oh!

openshift-merge-robot commented Aug 6, 2017

Uh oh!

openshift-bot commented Aug 7, 2017

Uh oh!

openshift-merge-robot commented Aug 7, 2017

Uh oh!

openshift-merge-robot commented Aug 7, 2017

Uh oh!

Uh oh!

bparees commented Aug 2, 2017 •

edited

Loading

coreydaley commented Aug 3, 2017 •

edited

Loading