add support for optional sync flag to block on async ops till completion #70
Conversation
app/request.go
Outdated
| if err := PrintProto(status); err != nil { | ||
| return err | ||
| } | ||
| fmt.Sprintf("started %s operation with requestId='%s', to monitor its progress use command: `%s request get -r '%s'`", operation, status.RequestId, AppName, status.RequestId) |
There was a problem hiding this comment.
[Nit] is this going to break anybody who has written scripts based on the output of commands from before? That could be annoying, maybe we don't print this to standard out or we have an option to suppress this?
There was a problem hiding this comment.
+1 we could check if stdout isatty and also have an env flag for disabling this (non-interactive mode maybe?)
There was a problem hiding this comment.
I like the idea to use isatty, to disable such messages.
I think we can leverage such mechanism to hide other messaging we do.
Let me address these in the next PR.
| } | ||
|
|
||
| func NewRequestCommand(getRequestClientFn GetRequestClientFn) (CommandOut, error) { | ||
|
|
||
| var c *RequestClient | ||
| return CommandOut{Command: &cli.Command{ | ||
| Name: "request", | ||
| Usage: "Manage asynchronous requests", | ||
| Usage: "Manage requests", |
There was a problem hiding this comment.
I wonder if long-running requests could more clear for the user.
There was a problem hiding this comment.
long-running seems technically wrong, because some write operations we do will be very quick.
There was a problem hiding this comment.
Async operation makes sense as it is inline with the new APIs
| @@ -60,14 +85,108 @@ func NewRequestCommand(getRequestClientFn GetRequestClientFn) (CommandOut, error | |||
| Flags: []cli.Flag{ | |||
| &cli.StringFlag{ | |||
| Name: "request-id", | |||
| Usage: "The request-id of the asynchronous request", | |||
| Usage: "The request-id of the request", | |||
There was a problem hiding this comment.
The request's ID could be less circular, or Unique identifier of the request
There was a problem hiding this comment.
actually we should probably change this to operation id. to keep things in consistent with the new apis
will address this in the next PR
| Usage: "wait for the request complete", | ||
| Aliases: []string{"w"}, | ||
| Flags: []cli.Flag{ | ||
| &cli.StringFlag{ |
There was a problem hiding this comment.
We could define this as a var since it is a repeated flag
| func (c *RequestClient) waitOnRequest(ctx *cli.Context, operation string, requestID string) error { | ||
|
|
||
| ticker := time.NewTicker(time.Millisecond) | ||
| timer := time.NewTimer(ctx.Duration(RequestTimeoutFlagName)) |
There was a problem hiding this comment.
Defer calls for the ticker and timer are missing.
app/request.go
Outdated
| fmt.Fprintf(writer, "waiting for request with id='%s' to finish, current state: %s\n", | ||
| requestID, request.State_name[int32(status.State)]) | ||
| } | ||
| ticker.Reset(time.Second * time.Duration(status.CheckDuration.Seconds)) |
There was a problem hiding this comment.
If status.CheckDuration.Seconds is 0 ticker.Reset will panic. Should we set a minimum value to ensure that this never happens and ensure that the ticker fires less often than 1ms?
app/account.go
Outdated
| return err | ||
| } | ||
| return r.HandleRequestStatus(ctx, "disable metrics", status) | ||
|
|
app/request.go
Outdated
| if err := PrintProto(status); err != nil { | ||
| return err | ||
| } | ||
| fmt.Sprintf("started %s operation with requestId='%s', to monitor its progress use command: `%s request get -r '%s'`", operation, status.RequestId, AppName, status.RequestId) |
There was a problem hiding this comment.
+1 we could check if stdout isatty and also have an env flag for disabling this (non-interactive mode maybe?)
| @@ -10,7 +10,7 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| AutoConfirmFlagName = "auto_confirm" | |||
| AutoConfirmFlagName = "auto-confirm" | |||
There was a problem hiding this comment.
I think should keep this as auto_confirm since folks are using it now. That or we could print a helpful message asking them to use auto-confirm instead - but that could break some use-cases folks have.
There was a problem hiding this comment.
added an alias to the flag, to support both.
app/request.go
Outdated
| for { | ||
| select { | ||
| case <-timer.C: | ||
| return fmt.Errorf("timed out waiting for request to complete, namespace=%s, requestID=%s, timeout=%s", |
There was a problem hiding this comment.
For some requests namespace could be empty right? If so I think we should conditionally add the field.
There was a problem hiding this comment.
yup, removed namespace.
app/request.go
Outdated
| return fmt.Errorf("request was cancelled: %s", status.FailureReason) | ||
| } | ||
| if operation != "" { | ||
| fmt.Fprintf(writer, "waiting for %s operation (requestId='%s') to finish, current state: %s\n", |
There was a problem hiding this comment.
id='%s' to keep it consistent with the other log statements?
| if err := PrintProto(status); err != nil { | ||
| return err | ||
| } | ||
| fmt.Printf( |
There was a problem hiding this comment.
I think we should allow these messages to be silenced for scripted use cases which expect JSON output on success. Perhaps isatty gets us this in the follow-up PR? Unsure if it covers all cases or not.
There was a problem hiding this comment.
I plan on addressing this comprehensively in the next PR.
In the next PR i hope to add a tcld version checker, which notifies the user to update their tcld when it gets old.
No description provided.