Return error message when using --untag with nonexistent tag #880

danielhelfand · 2020-06-08T19:22:29Z

Description

This pull request returns an error message when using kn service update serviceName --untag tagName --no-wait on a tag that does not exist.

The error returned by kn will be as follows, but I am open to any change with regard to the message:

Error: tag tagName does not exist

In the event multiple nonexistent tags are specified, kn will return multiple errors:

kn service update serviceName --untag tagName --untag tagName2 --no-wait

Error: tag tagName does not exist
Error: tag tagName2 does not exist

If a tag that does exist is specified with a nonexistent tag, the error will be displayed and no update will be made.

Please let me know if this should warrant an e2e test as well.

Reference

Fixes #874

/lint

knative-prow-robot

@danielhelfand: 0 warnings.

In response to this:

Description

This pull request returns an error message when using kn service update serviceName --untag tagName on a tag that does not exist.

The error returned by kn will be as follows, but I am open to any change with regard to the message:
Error: tag tagName does not exist
In the event multiple nonexistent tags are specified, kn will return multiple errors:
kn service update serviceName --untag tagName --untag tagName2

Error: tag tagName does not exist
Error: tag tagName2 does not exist
If a tag that does exist is specified with a nonexistent tag, the error will be displayed and no update will be made.

Please let me know if this should warrant an e2e test as well.

Reference

Fixes #874

/lint

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.

knative-prow-robot · 2020-06-08T19:22:42Z

Hi @danielhelfand. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 · 2020-06-09T06:07:45Z

/ok-to-test

rhuss · 2020-06-09T06:14:37Z

pkg/kn/commands/service/update_test.go

+	_, _, _, err := fakeServiceUpdate(orig, []string{
+		"service", "update", "foo", "--untag", "foo", "--no-wait"})
+
+	assert.Error(t, err, "Error: tag foo does not exist")


I'd suggest to change this to (please add the import, too, if missing0:

assert.Assert(t, util.ContainsAll(err.Error(), "tag", "foo", "not exist"))

It's always very fragile to assert on the exact wording of something, which might slight improvements tedious to work on (as then always a test needs to be fixed). Instead, just check that the important context information is contained in the error message. This focus also having good error message which as much context information as possible to help a user to understand and fix the error. E.g. Here you could also add the service name which would help the user to understand which service you wanted to tag. The more context, the better.

Also, please use error message according to the golang standard: Starting lower case, no punctuation at the need. The "Error:" prefix is also redundant here (its an error anyway), and in fact the top-level routine which prints the error will add the error prefix anyway. So better remove it here, too.

We have an issues open (not sure where), where we want to check for all error messages to be consistently following this rule. Might be a good exercise to automate that and make it part of the build process.

Just to clarify here, the error: prefix won't be added unless included in the message it seems:

./kn service update hello --untag foo --no-wait tag(s) foo not present for any revisions of service hello

If you remove silenceErrors, Cobra will handle errors without needing to print the error in main.go as is done now:

./kn service update hello --untag foo Error: tag(s) foo not present for any revisions of service hello

I think it makes sense to revisit this by changing how kn handles errors.

Totally agree, that a consistent error handling is missing, but for consistency's sake, I think there should be a single point which prepares an error message for print out.

In fact, in the refactoring in #877 there is a single exit point in case of a custom error, that now adds the appropriate prefix -->

client/cmd/kn/main.go

Lines 38 to 41 in a71cc3f

if err != nil {

fmt.Fprintf(os.Stderr, "Error: %v\n", err)

// This is the only point from where to exit when an error occurs

os.Exit(1)

We can more error messaging mangling at this point (like uppercasing the first letter), but if we assume that the change above gets merged, then we should already avoid an Error: prefix.

But don't let that block this PR as we have to go over all error messages later anyway.

navidshaikh

thanks @danielhelfand !
looks good to me!
We should add e2e test.

rhuss

Thanks ! looks good, but I have some suggestion regarding the wording of the error messages.

pkg/kn/traffic/compute.go

pkg/kn/traffic/compute_test.go

danielhelfand · 2020-06-09T14:13:56Z

/retest

pkg/kn/commands/service/update.go

pkg/kn/traffic/compute.go

rhuss

Thanks a lot !

Looks good to me, but as @navidshaikh mentioned we should add an extra e2e test for this error handling to https://github.com/knative/client/blob/master/test/e2e/traffic_split_test.go and also an entry to https://github.com/knative/client/blob/master/CHANGELOG.adoc

navidshaikh

suggested minor nit for return value from function

pkg/kn/traffic/compute.go

danielhelfand · 2020-06-10T19:06:37Z

/retest

knative-metrics-robot · 2020-06-10T19:09:34Z

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/update.go	85.5%	90.9%	5.5

danielhelfand · 2020-06-10T19:21:56Z

Thanks a lot !

Looks good to me, but as @navidshaikh mentioned we should add an extra e2e test for this error handling to https://github.com/knative/client/blob/master/test/e2e/traffic_split_test.go and also an entry to https://github.com/knative/client/blob/master/CHANGELOG.adoc

Added e2e tests to verify output of error message under service_test.go as well as state of revisions when error occurs under traffic_split_test.go. Also made CHANGELOG entry.

danielhelfand · 2020-06-10T19:36:57Z

/test pull-knative-client-integration-tests

rhuss

thanks !

/lgtm
/approve

knative-prow-robot · 2020-06-11T06:21:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielhelfand, navidshaikh, rhuss

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [navidshaikh,rhuss]

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

…#880) * return error message from using --untag with nonexistent tag * improve err message for --untag and add e2e test * change untagRevision to return bool and add traffic e2e tests * update changelog

- Updates the vendor/modules.txt - Update CHANGELOG - Squashed commits for knative#866 knative#880

- Updates the vendor/modules.txt - Update CHANGELOG - Squashed commits for knative#866 knative#880 knative#883

- Updates the vendor/modules.txt - Update CHANGELOG - Squashed commits for #866 #880 #883

return error message from using --untag with nonexistent tag

4e785cf

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

knative-prow-robot requested review from cppforlife and evankanderson June 8, 2020 19:22

knative-prow-robot reviewed Jun 8, 2020

View reviewed changes

knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 8, 2020

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 9, 2020

rhuss reviewed Jun 9, 2020

View reviewed changes

navidshaikh approved these changes Jun 9, 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 Jun 9, 2020

rhuss requested changes Jun 9, 2020

View reviewed changes

pkg/kn/traffic/compute.go Outdated Show resolved Hide resolved

pkg/kn/traffic/compute_test.go Outdated Show resolved Hide resolved

danielhelfand force-pushed the issue_874 branch 3 times, most recently from e957ff8 to 1baee91 Compare June 9, 2020 14:43

improve err message for --untag and add e2e test

07a0e2f

danielhelfand force-pushed the issue_874 branch from 1baee91 to 07a0e2f Compare June 9, 2020 14:59

danielhelfand commented Jun 9, 2020

View reviewed changes

pkg/kn/commands/service/update.go Show resolved Hide resolved

danielhelfand commented Jun 9, 2020

View reviewed changes

pkg/kn/traffic/compute.go Show resolved Hide resolved

rhuss requested changes Jun 10, 2020

View reviewed changes

navidshaikh reviewed Jun 10, 2020

View reviewed changes

pkg/kn/traffic/compute.go Outdated Show resolved Hide resolved

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 10, 2020

mattmoor reviewed Jun 10, 2020

View reviewed changes

danielhelfand force-pushed the issue_874 branch 2 times, most recently from 94d3895 to 3ae19db Compare June 10, 2020 16:33

danielhelfand force-pushed the issue_874 branch from 3ae19db to 79dc9cb Compare June 10, 2020 17:19

change untagRevision to return bool and add traffic e2e tests

de2c466

danielhelfand force-pushed the issue_874 branch from 79dc9cb to de2c466 Compare June 10, 2020 18:00

update changelog

4903bde

rhuss approved these changes Jun 11, 2020

View reviewed changes

knative-prow-robot assigned rhuss Jun 11, 2020

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

knative-prow-robot merged commit 3f146b1 into knative:master Jun 11, 2020

navidshaikh added the backport/candidate Consider this PR to be backported to the release branch label Jun 15, 2020

navidshaikh changed the title ~~Return Error Message from Using --untag with Nonexistent Tag~~ Return error message when using --untag with nonexistent tag Jun 16, 2020

navidshaikh mentioned this pull request Jun 16, 2020

v0.15.2 release backports #886

Merged

navidshaikh pushed a commit to navidshaikh/client that referenced this pull request Jun 16, 2020

Backports for client v0.15.2 release

9727fdc

- Updates the vendor/modules.txt - Update CHANGELOG - Squashed commits for knative#866 knative#880

navidshaikh added a commit to navidshaikh/client that referenced this pull request Jun 16, 2020

Backports for client v0.15.2 release

a02b444

- Updates the vendor/modules.txt - Update CHANGELOG - Squashed commits for knative#866 knative#880

navidshaikh added a commit to navidshaikh/client that referenced this pull request Jun 16, 2020

Backports for client v0.15.2 release

88f15b4

- Updates the vendor/modules.txt - Update CHANGELOG - Squashed commits for knative#866 knative#880 knative#883

knative-prow-robot pushed a commit that referenced this pull request Jun 16, 2020

Backports for client v0.15.2 release (#886)

efc9df1

- Updates the vendor/modules.txt - Update CHANGELOG - Squashed commits for #866 #880 #883

navidshaikh added backported-to/0.15 and removed backport/candidate Consider this PR to be backported to the release branch labels Aug 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return error message when using --untag with nonexistent tag #880

Return error message when using --untag with nonexistent tag #880

danielhelfand commented Jun 8, 2020 •

edited

Loading

knative-prow-robot left a comment

knative-prow-robot commented Jun 8, 2020

rhuss commented Jun 9, 2020

rhuss Jun 9, 2020

danielhelfand Jun 9, 2020 •

edited

Loading

rhuss Jun 10, 2020

rhuss Jun 10, 2020

navidshaikh left a comment

rhuss left a comment

danielhelfand commented Jun 9, 2020

rhuss left a comment

navidshaikh left a comment

danielhelfand commented Jun 10, 2020

knative-metrics-robot commented Jun 10, 2020

danielhelfand commented Jun 10, 2020 •

edited

Loading

danielhelfand commented Jun 10, 2020

rhuss left a comment

knative-prow-robot commented Jun 11, 2020

	if err != nil {
	fmt.Fprintf(os.Stderr, "Error: %v\n", err)
	// This is the only point from where to exit when an error occurs
	os.Exit(1)

Return error message when using --untag with nonexistent tag #880

Return error message when using --untag with nonexistent tag #880

Conversation

danielhelfand commented Jun 8, 2020 • edited Loading

Description

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Reference

knative-prow-robot commented Jun 8, 2020

rhuss commented Jun 9, 2020

rhuss Jun 9, 2020

Choose a reason for hiding this comment

danielhelfand Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

rhuss Jun 10, 2020

Choose a reason for hiding this comment

rhuss Jun 10, 2020

Choose a reason for hiding this comment

navidshaikh left a comment

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

danielhelfand commented Jun 9, 2020

rhuss left a comment

Choose a reason for hiding this comment

navidshaikh left a comment

Choose a reason for hiding this comment

danielhelfand commented Jun 10, 2020

knative-metrics-robot commented Jun 10, 2020

danielhelfand commented Jun 10, 2020 • edited Loading

danielhelfand commented Jun 10, 2020

rhuss left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jun 11, 2020

danielhelfand commented Jun 8, 2020 •

edited

Loading

danielhelfand Jun 9, 2020 •

edited

Loading

danielhelfand commented Jun 10, 2020 •

edited

Loading