refactor(service): Add a ProgressHandler for the Wait interface #234
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
knative-prow-robot
requested review from
jonjohnsonjr,
mattmoor,
mdemirhan,
n3wscott and
sixolet
knative-prow-robot
added
the
size/XXL
|
/hold |
knative-prow-robot
added
the
do-not-merge/hold
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.
rhuss
force-pushed
the
pr/wait-progresshandler
branch
from
3ec5ea6
to
f8cd4be
Compare
rhuss
removed
the
do-not-merge/hold
knative-prow-robot
added
size/L
|
The following is the coverage report on pkg/.
|
|
/retest |
| // 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) |
There was a problem hiding this comment.
How about UpdateProgress instead of Tic? Or at least Tick?
|
|
||
| // Printout ERROR label, but not the error. | ||
| // The error will be printed out later anyway | ||
| func (w *simpleProgressHandler) Fail(err error) { |
There was a problem hiding this comment.
Unsure of this code. I feel like the contract "the progress handler will print an ERROR label but you-the-caller have to fill out the rest of the line" kind of splits responsibilities uncomfortably. Do we have any other reasonable options?
There was a problem hiding this comment.
Tbh, the handler is free to do what it wants to do with the error (so the documentation is misleading and explains the default implementation).
The contract is, that this method will be called when error occurs and the WaitForServce() method will return this error.
Thinking about this, maybe we don't even need the ProgressHandler as success/error can be easily handled based on the return value of the method (it's a synchronous method). Only for the progress indication (which we currently don't really use), we would need the callback, but the start and end indicators can be completely handled outside the method.
Let me check, how I can make that simpler.
This puts all the console output to the command, where it belongs too. One could add a ProgressHandler for more fine granular feedback (like suggested in knative#234) but for now this is not needed.
|
I'm closing this PR in favour of #248 which is much simpler and achieves the same: Removing In case we ever need a ProgressHandler to print out real progress (like watching and printing the various phases during startup), we could reintroduce this interface later again. For now its just overkill. Thanks for the heads-up, @sixolet |
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.
This commit relies on that being PR #134 merged first.