From f8cd4be235331eb4dc3f40aa1bcb3b07006d9f41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Mon, 20 May 2019 21:11:17 +0200 Subject: [PATCH] refactor(service): Add a ProgressHandler for the Wait interface This remove the io.Writer from the client interface and replaces it with ProgressHandler interface. A SimpleProgressHandler instance can be used to print out the wait progress on a writer. --- docs/cmd/kn.md | 5 +- .../commands/revision/revision_list_test.go | 1 + pkg/kn/commands/service/service.go | 16 ++++ pkg/kn/commands/service/service_create.go | 5 +- pkg/kn/commands/service/wait_args.go | 43 --------- pkg/serving/v1alpha1/client.go | 11 ++- pkg/serving/v1alpha1/client_test.go | 4 +- pkg/wait/progress_handler.go | 87 +++++++++++++++++++ pkg/wait/progress_handler_test.go | 44 ++++++++++ pkg/wait/wait_for_ready.go | 31 ++++--- pkg/wait/wait_for_ready_test.go | 5 +- 11 files changed, 183 insertions(+), 69 deletions(-) delete mode 100644 pkg/kn/commands/service/wait_args.go create mode 100644 pkg/wait/progress_handler.go create mode 100644 pkg/wait/progress_handler_test.go diff --git a/docs/cmd/kn.md b/docs/cmd/kn.md index 627c15a5d5..f172ab627d 100644 --- a/docs/cmd/kn.md +++ b/docs/cmd/kn.md @@ -6,8 +6,9 @@ Knative client Manage your Knative building blocks: -* [Serving](https://github.com/knative/serving/tree/master): Manage your services and release new software to them. -* [Eventing](https://github.com/knative/eventing/tree/master): Manage event subscriptions and channels. Connect event sources. +Serving: Manage your services and release new software to them. +Build: Create builds and keep track of their results. +Eventing: Manage event subscriptions and channels. Connect up event sources. ### Options diff --git a/pkg/kn/commands/revision/revision_list_test.go b/pkg/kn/commands/revision/revision_list_test.go index af0b86c2f5..ff82a0f8fb 100644 --- a/pkg/kn/commands/revision/revision_list_test.go +++ b/pkg/kn/commands/revision/revision_list_test.go @@ -26,6 +26,7 @@ import ( client_testing "k8s.io/client-go/testing" "github.com/knative/client/pkg/kn/commands" + "github.com/knative/client/pkg/util" ) diff --git a/pkg/kn/commands/service/service.go b/pkg/kn/commands/service/service.go index 7a4ff8ef2a..6b00ea54d1 100644 --- a/pkg/kn/commands/service/service.go +++ b/pkg/kn/commands/service/service.go @@ -15,7 +15,12 @@ package service import ( + "fmt" + "io" + "github.com/knative/client/pkg/kn/commands" + "github.com/knative/client/pkg/wait" + "github.com/spf13/cobra" ) @@ -31,3 +36,14 @@ func NewServiceCommand(p *commands.KnParams) *cobra.Command { serviceCmd.AddCommand(NewServiceUpdateCommand(p)) return serviceCmd } + +// Create a new service progress handler for waiting on a service to become ready. +func newServiceWaitProgressHandler(stdout io.Writer, name string) wait.ProgressHandler { + return wait.NewSimpleProgressHandler( + stdout, + fmt.Sprintf("Waiting for service '%s' to become ready ... ", name), + ".", + "ERROR", + "OK", + ) +} diff --git a/pkg/kn/commands/service/service_create.go b/pkg/kn/commands/service/service_create.go index 80a6e60f57..0a28e80292 100644 --- a/pkg/kn/commands/service/service_create.go +++ b/pkg/kn/commands/service/service_create.go @@ -104,7 +104,10 @@ func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command { } if !waitFlags.Async { - err := client.WaitForService(name, time.Duration(waitFlags.TimeoutInSeconds)*time.Second, cmd.OutOrStdout()) + err := client.WaitForService( + name, + time.Duration(waitFlags.TimeoutInSeconds)*time.Second, + newServiceWaitProgressHandler(cmd.OutOrStdout(), name)) if err != nil { return err } diff --git a/pkg/kn/commands/service/wait_args.go b/pkg/kn/commands/service/wait_args.go deleted file mode 100644 index 8d985c26b6..0000000000 --- a/pkg/kn/commands/service/wait_args.go +++ /dev/null @@ -1,43 +0,0 @@ -// Copyright © 2019 The Knative Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package service - -import ( - "fmt" - - "github.com/knative/client/pkg/wait" - "github.com/knative/pkg/apis" - serving_v1alpha1_api "github.com/knative/serving/pkg/apis/serving/v1alpha1" - serving_v1alpha1_client "github.com/knative/serving/pkg/client/clientset/versioned/typed/serving/v1alpha1" - "k8s.io/apimachinery/pkg/runtime" -) - -// Create wait arguments for a Knative service which can be used to wait for -// a create/update options to be finished -// Can be used by `service_create` and `service_update`, hence this extra file -func newServiceWaitForReady(client serving_v1alpha1_client.ServingV1alpha1Interface, namespace string) wait.WaitForReady { - return wait.NewWaitForReady( - "service", - client.Services(namespace).Watch, - serviceConditionExtractor) -} - -func serviceConditionExtractor(obj runtime.Object) (apis.Conditions, error) { - service, ok := obj.(*serving_v1alpha1_api.Service) - if !ok { - return nil, fmt.Errorf("%v is not a service", obj) - } - return apis.Conditions(service.Status.Conditions), nil -} diff --git a/pkg/serving/v1alpha1/client.go b/pkg/serving/v1alpha1/client.go index 54aa905f2b..369258ff5e 100644 --- a/pkg/serving/v1alpha1/client.go +++ b/pkg/serving/v1alpha1/client.go @@ -16,7 +16,6 @@ package v1alpha1 import ( "fmt" - "io" "time" "github.com/knative/pkg/apis" @@ -52,8 +51,9 @@ type KnClient interface { // Delete a service by name DeleteService(name string) error - // Wait for a service to become ready, but not longer than provided timeout - WaitForService(name string, timeout time.Duration, out io.Writer) error + // Wait for a service to become ready, but not longer than provided timeout. + // Optionally a single progress handler can be given to print out progress + WaitForService(name string, timeout time.Duration, progressHandler ...wait.ProgressHandler) error // Get a revision by name GetRevision(name string) (*v1alpha1.Revision, error) @@ -188,9 +188,9 @@ func (cl *knClient) DeleteService(serviceName string) error { } // Wait for a service to become ready, but not longer than provided timeout -func (cl *knClient) WaitForService(name string, timeout time.Duration, out io.Writer) error { +func (cl *knClient) WaitForService(name string, timeout time.Duration, progressHandlers ...wait.ProgressHandler) error { waitForReady := newServiceWaitForReady(cl.client.Services(cl.namespace).Watch) - return waitForReady.Wait(name, timeout, out) + return waitForReady.Wait(name, timeout, progressHandlers...) } // Get a revision by name @@ -281,7 +281,6 @@ func updateServingGvk(obj runtime.Object) error { // Can be used by `service_create` and `service_update`, hence this extra file func newServiceWaitForReady(watch wait.WatchFunc) wait.WaitForReady { return wait.NewWaitForReady( - "service", watch, serviceConditionExtractor) } diff --git a/pkg/serving/v1alpha1/client_test.go b/pkg/serving/v1alpha1/client_test.go index 862e2ce116..5790d7e583 100644 --- a/pkg/serving/v1alpha1/client_test.go +++ b/pkg/serving/v1alpha1/client_test.go @@ -15,7 +15,6 @@ package v1alpha1 import ( - "bytes" "fmt" "testing" "time" @@ -357,8 +356,7 @@ func TestWaitForService(t *testing.T) { }) t.Run("wait on a service to become ready with success", func(t *testing.T) { - buf := new(bytes.Buffer) - err := client.WaitForService(serviceName, 60*time.Second, buf) + err := client.WaitForService(serviceName, 60*time.Second, wait.NoopProgressHandler{}) assert.NilError(t, err) }) } diff --git a/pkg/wait/progress_handler.go b/pkg/wait/progress_handler.go new file mode 100644 index 0000000000..d9fb350b70 --- /dev/null +++ b/pkg/wait/progress_handler.go @@ -0,0 +1,87 @@ +// Copyright © 2019 The Knative Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package wait + +import ( + "fmt" + "io" +) + +// A callback which is informed about the progress of an operation +type ProgressHandler interface { + // Called when the operation starts + Start() + + // Called with a percentage indicating completion + // The argument will be in the range 0..100 if the progress percentage can be calculated + // or -1 if only an ongoing operation should be indicated + Tic(complete int) + + // Called when operation finished sucessfully + Success() + + // Called when the operation failed with the erro + Fail(err error) +} + +// No operation progress handler which just stays silent +type NoopProgressHandler struct{} + +func (w NoopProgressHandler) Start() {} +func (w NoopProgressHandler) Tic(int) {} +func (w NoopProgressHandler) Fail(error) {} +func (w NoopProgressHandler) Success() {} + +// Standard progress handler for printing out progress to a given writer +type simpleProgressHandler struct { + out io.Writer + startLabel string + ticMark string + errorLabel string + successLabel string +} + +// Create a new progress handler which writes out to the given stream. +// The label will be printed right after start, error & success are terminal message depending on the outcome +// In case of an error, the error itself is not printed as it is supposed that the calling function +// will deal with the error and eventually print it out. +func NewSimpleProgressHandler(out io.Writer, label string, tic string, error string, success string) ProgressHandler { + return &simpleProgressHandler{out, label, tic, error, success} +} + +// Print our intial label +func (w *simpleProgressHandler) Start() { + fmt.Fprint(w.out, w.startLabel) +} + +// Tic progress +func (w *simpleProgressHandler) Tic(complete int) { + if complete < 0 { + fmt.Fprint(w.out, w.ticMark) + } else { + fmt.Fprintf(w.out, " %d%%", complete) + } +} + +// Printout ERROR label, but not the error. +// The error will be printed out later anyway +func (w *simpleProgressHandler) Fail(err error) { + fmt.Fprintln(w.out, w.errorLabel) +} + +// Printout success label +func (w *simpleProgressHandler) Success() { + fmt.Fprintln(w.out, w.successLabel) +} diff --git a/pkg/wait/progress_handler_test.go b/pkg/wait/progress_handler_test.go new file mode 100644 index 0000000000..f84415d23c --- /dev/null +++ b/pkg/wait/progress_handler_test.go @@ -0,0 +1,44 @@ +// Copyright © 2019 The Knative Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package wait + +import ( + "bytes" + "errors" + "strings" + "testing" + + "gotest.tools/assert" +) + +func TestSimpleProgressHandler(t *testing.T) { + buf := new(bytes.Buffer) + ph := NewSimpleProgressHandler(buf, "START", ".", "ERROR", "OK") + ph.Start() + assert.Assert(t, strings.Contains(buf.String(), "START")) + ph.Tic(-1) + assert.Assert(t, strings.Contains(buf.String(), ".")) + ph.Tic(-1) + assert.Assert(t, strings.Contains(buf.String(), "..")) + ph.Tic(8) + assert.Assert(t, strings.Contains(buf.String(), " 8%")) + ph.Tic(42) + assert.Assert(t, strings.Contains(buf.String(), " 8% 42%"), buf.String()) + ph.Fail(errors.New("foobar")) + assert.Assert(t, strings.Contains(buf.String(), "ERROR\n")) + assert.Assert(t, !strings.Contains(buf.String(), "foobar")) + ph.Success() + assert.Assert(t, strings.Contains(buf.String(), "OK\n"), buf.String()) +} diff --git a/pkg/wait/wait_for_ready.go b/pkg/wait/wait_for_ready.go index f4c41700f8..acd8b0ee33 100644 --- a/pkg/wait/wait_for_ready.go +++ b/pkg/wait/wait_for_ready.go @@ -31,7 +31,6 @@ import ( type waitForReadyConfig struct { watchFunc WatchFunc conditionsExtractor ConditionsExtractor - kind string } // Interface used for waiting of a resource of a given name to reach a definitive @@ -39,8 +38,8 @@ type waitForReadyConfig struct { type WaitForReady interface { // Wait on resource the resource with this name until a given timeout - // and write status out on writer - Wait(name string, timeout time.Duration, out io.Writer) error + // Use an optional progresshandler to indicate progress (only the first progress handler is used) + Wait(name string, timeout time.Duration, progressHandlers ...ProgressHandler) error } // Create watch which is used when waiting for Ready condition @@ -50,9 +49,8 @@ type WatchFunc func(opts v1.ListOptions) (watch.Interface, error) type ConditionsExtractor func(obj runtime.Object) (apis.Conditions, error) // Constructor with resource type specific configuration -func NewWaitForReady(kind string, watchFunc WatchFunc, extractor ConditionsExtractor) WaitForReady { +func NewWaitForReady(watchFunc WatchFunc, extractor ConditionsExtractor) WaitForReady { return &waitForReadyConfig{ - kind: kind, watchFunc: watchFunc, conditionsExtractor: extractor, } @@ -62,26 +60,37 @@ func NewWaitForReady(kind string, watchFunc WatchFunc, extractor ConditionsExtra // `watchFunc` creates the actual watch, `kind` is the type what your are watching for // (e.g. "service"), `timeout` is a timeout after which the watch should be cancelled if no // target state has been entered yet and `out` is used for printing out status messages -func (w *waitForReadyConfig) Wait(name string, timeout time.Duration, out io.Writer) error { +func (w *waitForReadyConfig) Wait(name string, timeout time.Duration, progressHandlers ...ProgressHandler) error { opts := v1.ListOptions{ FieldSelector: fields.OneTermEqualSelector("metadata.name", name).String(), } addWatchTimeout(&opts, timeout) - fmt.Fprintf(out, "Waiting for %s '%s' to become ready ... ", w.kind, name) - flush(out) + var ph ProgressHandler + switch len(progressHandlers) { + case 0: + ph = NoopProgressHandler{} + case 1: + ph = progressHandlers[0] + default: + return fmt.Errorf("only one progresshandler allowed, but found %d (internal error)", len(progressHandlers)) + } + + ph.Start() floatingTimeout := timeout for { start := time.Now() retry, timeoutReached, err := w.waitForReadyCondition(opts, name, floatingTimeout) if err != nil { - fmt.Fprintln(out) + ph.Fail(err) return err } floatingTimeout = floatingTimeout - time.Since(start) if timeoutReached || floatingTimeout < 0 { - return fmt.Errorf("timeout: %s '%s' not ready after %d seconds", w.kind, name, int(timeout/time.Second)) + err := fmt.Errorf("timeout: '%s' not ready after %d seconds", name, int(timeout/time.Second)) + ph.Fail(err) + return err } if retry { @@ -89,7 +98,7 @@ func (w *waitForReadyConfig) Wait(name string, timeout time.Duration, out io.Wri continue } - fmt.Fprintln(out, "OK") + ph.Success() return nil } } diff --git a/pkg/wait/wait_for_ready_test.go b/pkg/wait/wait_for_ready_test.go index e27984fdfe..4e96564158 100644 --- a/pkg/wait/wait_for_ready_test.go +++ b/pkg/wait/wait_for_ready_test.go @@ -42,7 +42,6 @@ func TestAddWaitForReady(t *testing.T) { outBuffer := new(bytes.Buffer) waitForReady := NewWaitForReady( - "blub", func(opts v1.ListOptions) (watch.Interface, error) { return fakeWatchApi, nil }, @@ -50,7 +49,7 @@ func TestAddWaitForReady(t *testing.T) { return apis.Conditions(obj.(*v1alpha1.Service).Status.Conditions), nil }) fakeWatchApi.Start() - err := waitForReady.Wait("foobar", tc.timeout, outBuffer) + err := waitForReady.Wait("foobar", tc.timeout, NewSimpleProgressHandler(outBuffer, "foobar", ".", "ERROR", "OK")) close(fakeWatchApi.eventChan) if !tc.errorExpected && err != nil { @@ -81,7 +80,7 @@ func TestAddWaitForReady(t *testing.T) { // Test cases which consists of a series of events to send and the expected behaviour. func prepareTestCases(name string) []waitForReadyTestCase { return []waitForReadyTestCase{ - {peNormal(name), time.Second, false, []string{"OK", "foobar", "blub"}}, + {peNormal(name), time.Second, false, []string{"OK", "foobar"}}, {peError(name), time.Second, true, []string{"FakeError"}}, {peTimeout(name), time.Second, true, []string{"timeout"}}, {peWrongGeneration(name), time.Second, true, []string{"timeout"}},