kops: Don't try to split --set flags on commas

[justinsb]

Passing quotes in very difficult with the --set flag, because the
pflag library tries to parse it using strict CSV semantics. We switch
to using the StringArrayVar, which does not attempt splitting.

This is a breaking change; compound flags must now be written as two flags, but I think that is clearer.

Example: --set=a,b must now be --set=a --set=b

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from justinsb. For more information see the Kubernetes Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hakman]

This would break some scenarios, like scalability tests.
/hold

[justinsb]

This would break some scenarios, like scalability tests.

Ah, bummer. I do think --set=foo=bar --set=foo2=bar2 is more readable than --set=foo=bar,foo2=bar2, particularly when the paths get long, but it is a breaking change.

We could also just do a more naive splitting on commas. We can also use the CSV splitter with LazyQuotes=true, this was proposed in spf13/pflag#371

[hakman]

I think just fixing the scalability scenario would be enough. It's not such a bad change.
There may be other problems with the way we de-duplicate args in kubetest (I guess it's why I did this ugly thing).

kops/tests/e2e/scenarios/scalability/run-test.sh

Line 89 in b4d787f

create_args=("--network-cidr=10.0.0.0/16,10.1.0.0/16,10.2.0.0/16,10.3.0.0/16,10.4.0.0/16,10.5.0.0/16,10.6.0.0/16,10.7.0.0/16,10.8.0.0/16,10.9.0.0/16,10.10.0.0/16,10.11.0.0/16,10.12.0.0/16")

[justinsb]

I do think you raise a good point though, and it's possible that people are setting compound flags "in the wild" that we don't know about, so I don't particularly want to break them if we can avoid it.

The fix is actually pretty easy, in #16128 - we just need to set LazyQuotes: true. It's a bit harder than it should be, but it would be very easy if pflag merged the support.

My 2c: #16128 unblocks us without breaking anyone...

[hakman]

#16128 was merged.
/close

[k8s-ci-robot]

@hakman: Closed this PR.

Details

In response to this:

#16128 was merged.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.