Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jun 11, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4e785cf
return error message from using --untag with nonexistent tag
danielhelfand Jun 8, 2020
07a0e2f
improve err message for --untag and add e2e test
danielhelfand Jun 9, 2020
de2c466
change untagRevision to return bool and add traffic e2e tests
danielhelfand Jun 10, 2020
4903bde
update changelog
danielhelfand Jun 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions pkg/kn/commands/service/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,15 @@ func TestServiceUpdateDeletionTimestampNotNil(t *testing.T) {
assert.ErrorContains(t, err, "service")
}

func TestServiceUpdateTagDoesNotExist(t *testing.T) {
orig := newEmptyService()

_, _, _, err := fakeServiceUpdate(orig, []string{
"service", "update", "foo", "--untag", "foo", "--no-wait"})

assert.Error(t, err, "Error: tag foo does not exist")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@danielhelfand danielhelfand Jun 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

func newEmptyService() *servingv1.Service {
ret := &servingv1.Service{
TypeMeta: metav1.TypeMeta{
Expand Down
17 changes: 14 additions & 3 deletions pkg/kn/traffic/compute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,15 @@ func (e ServiceTraffic) isTagPresent(tag string) bool {
return false
}

func (e ServiceTraffic) untagRevision(tag string) {
func (e ServiceTraffic) untagRevision(tag string) error {
for i, target := range e {
if target.Tag == tag {
e[i].Tag = ""
break
return nil
}
}

return fmt.Errorf("Error: tag %s does not exist", tag)
}

func (e ServiceTraffic) isRevisionPresent(revision string) bool {
Expand Down Expand Up @@ -276,8 +278,17 @@ func Compute(cmd *cobra.Command, targets []servingv1.TrafficTarget, trafficFlags
traffic := newServiceTraffic(targets)

// First precedence: Untag revisions
var errStrings []string
for _, tag := range trafficFlags.UntagRevisions {
traffic.untagRevision(tag)
err = traffic.untagRevision(tag)
if err != nil {
errStrings = append(errStrings, err.Error())
}
}

// Return all errors from untagging revisions
if len(errStrings) > 0 {
return nil, fmt.Errorf(strings.Join(errStrings, "\n"))
rhuss marked this conversation as resolved.
Show resolved Hide resolved
}

for _, each := range trafficFlags.RevisionsTags {
Expand Down
12 changes: 12 additions & 0 deletions pkg/kn/traffic/compute_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,18 @@ func TestComputeErrMsg(t *testing.T) {
[]string{"--traffic", "echo-v1=40", "--traffic", "echo-v1=60"},
"repetition of revision reference echo-v1 is not allowed, use only once with --traffic flag",
},
{
"untag single tag that does not exist",
append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)),
[]string{"--untag", "foo"},
"Error: tag foo does not exist",
rhuss marked this conversation as resolved.
Show resolved Hide resolved
},
{
"untag multiple tags that do not exist",
append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("latest", "echo-v1", 100, false)),
[]string{"--untag", "foo", "--untag", "bar"},
"Error: tag foo does not exist\nError: tag bar does not exist",
},
} {
t.Run(testCase.name, func(t *testing.T) {
testCmd, tFlags := newTestTrafficCommand()
Expand Down