Handle map-like options consistently#592
Conversation
|
Hi @MIBc. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
knative-prow-robot
added
needs-ok-to-test
|
/ok-to-test |
knative-prow-robot
added
ok-to-test
|
/test pull-knative-client-go-coverage |
sixolet
changed the title
| } | ||
| filters, err := util.MapFromArray(f.Filters, "=") | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "invalid filter") |
There was a problem hiding this comment.
| return nil, errors.Wrap(err, "invalid filter") | |
| return nil, errors.Wrap(err, "Invalid --filter") |
| } | ||
| filters, err := util.MapFromArrayAllowingSingles(f.Filters, "=") | ||
| if err != nil { | ||
| return nil, nil, errors.Wrap(err, "invalid filter") |
There was a problem hiding this comment.
| return nil, nil, errors.Wrap(err, "invalid filter") | |
| return nil, nil, errors.Wrap(err, "Invalid --filter") |
|
The following is the coverage report on the affected files.
|
navidshaikh
left a comment
navidshaikh
left a comment
There was a problem hiding this comment.
/lgtm
/approve
Thank you!
@rhuss : The PR covers all the cases from #577 except the one, to error out if asked to remove an ENV during create, IMO it could be a valid use case #577 (comment), besides I've updated the PR description to not close the issue.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MIBc, navidshaikh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
* Update RPM spec file * Update spec file changelog date Co-authored-by: Navid Shaikh <nshaikh@redhat.com> Co-authored-by: Navid Shaikh <nshaikh@redhat.com>
6 participants
From the description of #577:
The behaviour should be (using --env as an example):
--env a1=b1 --env a2=b2 should add both env variables
--env a1 should set a1 to empty string
--env a1- should remove the environment variable for update and an error for create
--env a1=b1 --env a1=b2 should be an error as the same key can not be specified on the same command line
Implements all the cases mentioned in #577, except one being discussed.