Report an error if no flag(s) set in service update #318

zhanggbj · 2019-07-30T13:10:45Z

For now if no flag(s) set, service update will still try to do an update as below, it should return an error instead.

$ ./kn service update hello
Waiting for service 'hello' to become ready ... OK
Service 'hello' updated in namespace 'default'.

Fixes issue 286

Proposed Changes

Return an error if no flag(s) set in service update

Release Note

knative-prow-robot

@zhanggbj: 1 warning.

In response to this:

For now if no flag(s) set, service update will still try to
do an update, it should return an error instead.

issue 286

Fixes #

Proposed Changes

Release Note

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.

pkg/kn/commands/service/service_update.go

knative-prow-robot · 2019-07-30T13:10:59Z

Hi @zhanggbj. 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.

toVersus · 2019-07-30T14:14:14Z

pkg/kn/commands/service/service_update_test.go

+		t.Errorf("Unexpected action if no flag(s) set")
+	}
+
+	if err != nil {


I think you can reduce a if-else depth by swapping the if-else block and using t.Fatal.

@toVersus I addressed it, thanks!

maximilien · 2019-07-30T20:49:10Z

pkg/kn/commands/service/service_update.go

@@ -50,6 +50,10 @@ func NewServiceUpdateCommand(p *commands.KnParams) *cobra.Command {
 				return err
 			}

+			if cmd.Flags().NFlag() == 0 {


The other option would be to do this in a PreRun function which should allow separation of the checks to the Run. See this comment from Cobra community.

Since this check is simple, it's likely OK here but if we start adding more such checks or other commands then PreRun could be a nice way to write these pre-condition code. Cheers :)

@maximilien As you mentioned, there may be more checks needed later, and people even mentioned about the odd error message when wrong args here #317, so I moved this part to PreRun. Thanks!

maximilien

/ok-to-test

For now if no flag(s) set, service update will still try to do an update, it should return an error instead. [issue 286](knative#286)

knative-metrics-robot · 2019-07-31T08:21:23Z

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

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service/service_update.go	73.7%	76.2%	2.5

toVersus

LGTM, thanks!

knative-prow-robot · 2019-07-31T11:10:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, toVersus, zhanggbj

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~~ [maximilien]

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

maximilien · 2019-07-31T20:26:08Z

Thanks @zhanggbj and @toVersus for the review

/lgtm

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 30, 2019

knative-prow-robot requested review from cppforlife and rhuss July 30, 2019 13:10

knative-prow-robot reviewed Jul 30, 2019

View reviewed changes

pkg/kn/commands/service/service_update.go Outdated Show resolved Hide resolved

knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 30, 2019

zhanggbj force-pushed the service_update_input branch 2 times, most recently from bb0b714 to 73c16c1 Compare July 30, 2019 14:00

toVersus reviewed Jul 30, 2019

View reviewed changes

maximilien reviewed Jul 30, 2019

View reviewed changes

maximilien approved these changes Jul 30, 2019

View reviewed changes

zhanggbj force-pushed the service_update_input branch 3 times, most recently from 73c16c1 to dd02b81 Compare July 31, 2019 08:15

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 Jul 31, 2019

Report an error if no flag(s) set in service update

d4fcd1f

For now if no flag(s) set, service update will still try to do an update, it should return an error instead. [issue 286](knative#286)

zhanggbj force-pushed the service_update_input branch from dd02b81 to d4fcd1f Compare July 31, 2019 08:19

toVersus approved these changes Jul 31, 2019

View reviewed changes

knative-prow-robot assigned maximilien Jul 31, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2019

knative-prow-robot merged commit 8fbb51f into knative:master Jul 31, 2019

navidshaikh pushed a commit to navidshaikh/client that referenced this pull request Apr 8, 2020

Removing myself from owners/reviewers (knative#318)

75b6af4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report an error if no flag(s) set in service update #318

Report an error if no flag(s) set in service update #318

zhanggbj commented Jul 30, 2019 •

edited

Loading

knative-prow-robot left a comment

knative-prow-robot commented Jul 30, 2019

toVersus Jul 30, 2019

zhanggbj Jul 31, 2019

maximilien Jul 30, 2019

zhanggbj Jul 31, 2019 •

edited

Loading

maximilien left a comment

knative-metrics-robot commented Jul 31, 2019

toVersus left a comment

knative-prow-robot commented Jul 31, 2019

maximilien commented Jul 31, 2019

Report an error if no flag(s) set in service update #318

Report an error if no flag(s) set in service update #318

Conversation

zhanggbj commented Jul 30, 2019 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

knative-prow-robot commented Jul 30, 2019

toVersus Jul 30, 2019

Choose a reason for hiding this comment

zhanggbj Jul 31, 2019

Choose a reason for hiding this comment

maximilien Jul 30, 2019

Choose a reason for hiding this comment

zhanggbj Jul 31, 2019 • edited Loading

Choose a reason for hiding this comment

maximilien left a comment

Choose a reason for hiding this comment

knative-metrics-robot commented Jul 31, 2019

toVersus left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jul 31, 2019

maximilien commented Jul 31, 2019

zhanggbj commented Jul 30, 2019 •

edited

Loading

zhanggbj Jul 31, 2019 •

edited

Loading