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.

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

Add current operation info to stack command output. #3822

Merged
merged 2 commits into from
Jan 30, 2020
Merged

Conversation

jkisk
Copy link
Contributor

@jkisk jkisk commented Jan 27, 2020

This will complete the process of wiring through current operation in progress info to the CLI, addressing issue #4468. If an update is currently in progress a "Last Updated" time won't be displayed.
Example:
image

@jkisk jkisk requested a review from chrsmith January 27, 2020 23:24
@jkisk jkisk changed the title Return current operation info Add current operation info to stack command output. Jan 28, 2020
@lukehoban
Copy link
Member

@jkisk Do you have an example of what this looks like that you could share?

Should we include a link to the update in the console?

@jkisk
Copy link
Contributor Author

jkisk commented Jan 28, 2020

I'm very open to feedback on how this should be formatted, or if we should add a link to the update in the console.

image

Copy link
Contributor

@chrsmith chrsmith left a comment

Choose a reason for hiding this comment

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

Thanks for attaching a screen shot.

Displaying both the "Last update" and "Operation in progress" data seems redundant. Perhaps we just show either "Last updated: ..." or "Update in progress: ..."

As far as the specific data you are displaying, I'd suggest against the phrase "operation in progress". As that doesn't sound super friendly. Also, what do you think about putting the start time first, as I imagine that's the most important piece of data someone wants to know. Finally, while wiring through the type of update being performed... I can't think of a natural way to mention that. e.g. "Update in progress: update" is super-confusing. So maybe we just don't display that information?

So putting that all together, I'd propose this alternative:

...
Managed by https://api.jamie.pulumi-dev.io
Owner: Kinkead
Update in progress:
    Started: 5 seconds ago
    Requested By: Kinkead
Pulumi version: ...
...

Also a big +1 to Luke's suggestion to add a permalink to the Pulumi console. Though I don't know if we have all the necessary data. (We might not have the version of the stack update in progress, which we'd need to link to it. Jamie, you can see callers of b.CloudConsoleURL for how we render these permalink elsewhere.)

@@ -2,6 +2,8 @@ CHANGELOG
=========

## HEAD (unreleased)
- Add information about an in-flight operation to the stack command output, if applicable.
Copy link
Contributor

Choose a reason for hiding this comment

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

As you can see from the other edits to the CHANGELOG.md file, it's useful to link to the bug that is being fixed so someone can drill into the specific item to learn more.

cmd/stack.go Outdated
@@ -69,6 +70,13 @@ func newStackCmd() *cobra.Command {
if isCloud {
if cs, ok := s.(httpstate.Stack); ok {
fmt.Printf(" Owner: %s\n", cs.OrgName())
// If there is an in-flight operation, provide info.
if cs.CurrentOperation() != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid the redundant cs.CurrentOperation(), why don't we declare it as a new variable. Such as:

if currentOp := cs.CurrentOperation(); currentOp != nil {
    fmt.Printf("	Operation in progress: %s\n", currentOp.Kind)
}

IMHO, it is a lot easier to read that way.

@jkisk jkisk merged commit 9c0e902 into master Jan 30, 2020
@pulumi-bot pulumi-bot deleted the jkinkead/current-op branch January 30, 2020 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants