Fix --image flag to only allow single occurence by dsimansk · Pull Request #647 · knative/client

dsimansk · 2020-02-11T07:38:11Z

Fixes #533

Proposed Changes

Introduce new type singletonString for flag values that only allow single value assignment, otherwise return error
Fix --image flag with the new type.

Example:

➜  client git:(issue-533) kn service create svc-test --image gcr.io/knative-samples/helloworld-go --image gcr.io/foo/bar:barz --no-wait 
invalid argument "gcr.io/foo/bar:barz" for "--image" flag: value is already set

knative-prow-robot

@dsimansk: 0 warnings.

Details

In response to this:

Fixes #533

Proposed Changes

Introduce new type singletonString for flag values that only allow single value assignment, otherwise return error

Fix --image flag with the new type.

Example:
➜  client git:(issue-533) kn service create svc-test --image gcr.io/knative-samples/helloworld-go --image --image gcr.io/foo/bar:barz --no-wait
invalid argument "--image" for "--image" flag: value is already set

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.

rhuss

Nice ! That's the way to go, please add some unit tests, too.

See also below for a suggestion how to improve the error message.

rhuss · 2020-02-11T08:58:34Z

+
+func (s *singletonString) Set(val string) error {
+	if len(*s) > 0 {
+		return errors.New("value is already set")


I'm not sure how this "feels" like on the CLI. If this is the only error printed, then it might be confusing (especially when you have a large list of options). It would be better to actually print which argument was duplicated (i.e. --image here). I think you could implement that by using a type like

type uniqueStringArg struct { // which kind of argument it is kind string // Argument value value string } func newUniqueStringArg(kind string) uniqueStringArg { return uniqueStringArg{kind: kind} }

and then partially pre-initialize the ConfiguarionEditFlags like in

ceF = ConfigurationEditFlags{ Image: newUniqueStringArg("image") }

and then finally adapt the error message to include the kind.

Does this make sense ?

Actually, the error already contains the flag.

➜ client git:(issue-533) kn service create svc-test --image gcr.io/knative-samples/helloworld-go --image gcr.io/foo/bar:barz --no-wait invalid argument "gcr.io/foo/bar:barz" for "--image" flag: value is already set

Maybe a bit more descriptive error message to explain what is wrong with value already set?

Ah, if this is the case, then I'm fine with having the current solution.

However for the error message I woudl use something like "flag can be provided only once" (so from the POV of the user, not from the flag's POV)

OK, anyway I do like your suggested name uniqueStringArg a bit more. I'll change the name, error message and finally add some unit tests.

Reflected review feedback in c68fe1d and added unit tests in c990288.

rhuss

tests are looking good with some minor remark how they could be improved.

rhuss · 2020-02-11T13:04:32Z

+func TestServiceCreateWithMultipleImages(t *testing.T) {
+	_, _, _, err := fakeServiceCreate([]string{
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--image", "gcr.io/bar/foo:baz", "--no-wait"}, false, false)
+	assert.Error(t, err, "invalid argument \"gcr.io/bar/foo:baz\" for \"--image\" flag: can be provided only once")


What we also do typically in unit test to assert on the content of the error message: not the exact wording but that key context information is included. This helps us to ensure that the error message is meaningful, but the test is also as robust as possible so that the exact wording could be changed without breaking the tests.

We have an utility util.ContainsAll() which helps here. Check for other tests that use that method.

dsimansk · 2020-02-11T14:42:54Z

/retest

maximilien

Nice, thanks for the contribution 👍 letting others chime in and approve

LGTM

maximilien · 2020-02-11T18:26:52Z

 }

+// -- uniqueStringArg Value
+// Custom implementation of flag.Value interface to prevent multiple value assignment.


knative-metrics-robot · 2020-02-12T07:42:26Z

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service/configuration_edit_flags.go	83.0%	83.5%	0.5

rhuss

Thanks !

/lgtm

knative-prow-robot · 2020-02-12T11:11:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, maximilien, rhuss

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [maximilien,rhuss]

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

@matejvasek

/cc @matejvasek @gauron99 @Kaustubh-pande

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 11, 2020

knative-prow-robot reviewed Feb 11, 2020

View reviewed changes

knative-prow-robot requested review from cppforlife and rhuss February 11, 2020 07:38

knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 11, 2020

rhuss suggested changes Feb 11, 2020

View reviewed changes

knative-prow-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 Feb 11, 2020

dsimansk mentioned this pull request Feb 11, 2020

Does not reject multiple --image parameters #533

Closed

rhuss suggested changes Feb 11, 2020

View reviewed changes

maximilien approved these changes Feb 11, 2020

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2020

dsimansk force-pushed the issue-533 branch 2 times, most recently from cb1962f to 253cdce Compare February 12, 2020 07:30

dsimansk added 6 commits February 12, 2020 08:36

Fix --image flag to only allow single occurence

a3b4eea

Fix changelog link

71e12f9

Reflect feedback in error message and type name

d90fbaf

Add unit tests

d4756e7

Reflect unit tests improvement from feedback

84d1d9f

Fix unit tests due to changes in master

29fb37c

dsimansk force-pushed the issue-533 branch from 253cdce to 29fb37c Compare February 12, 2020 07:39

rhuss approved these changes Feb 12, 2020

View reviewed changes

knative-prow-robot assigned rhuss Feb 12, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2020

knative-prow-robot merged commit dd2dc97 into knative:master Feb 12, 2020

dsimansk pushed a commit to dsimansk/client that referenced this pull request Mar 25, 2021

Fix arm64 download in serve.py (knative#647)

02fb8de

dsimansk added a commit to dsimansk/client that referenced this pull request May 6, 2025

[release-v1.16] Bump go-git to v5.13 (knative#647)

b4f5654

/cc @matejvasek @gauron99 @Kaustubh-pande

Conversation

dsimansk commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

rhuss left a comment

Choose a reason for hiding this comment

Uh oh!

rhuss Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

dsimansk Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsimansk Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhuss Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

dsimansk Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

dsimansk Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

rhuss left a comment

Choose a reason for hiding this comment

Uh oh!

rhuss Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

dsimansk commented Feb 11, 2020

Uh oh!

maximilien left a comment

Choose a reason for hiding this comment

Uh oh!

maximilien Feb 11, 2020

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Feb 12, 2020

Uh oh!

rhuss left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Feb 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dsimansk commented Feb 11, 2020 •

edited

Loading

dsimansk Feb 11, 2020 •

edited

Loading

dsimansk Feb 11, 2020 •

edited

Loading