-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
show time elapsed per resource in update #10953
Conversation
Changelog[uncommitted] (2022-10-11)Features
|
7abb9c8
to
a331aa5
Compare
a331aa5
to
b1c0d1d
Compare
return "" | ||
end, ok := display.opStopwatch.end[step.URN] | ||
if !ok { | ||
return opText |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wander if we should add a bit that the timing information was missing? It shouldn't ever be hit but if it does might prompt people to tell us about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think silently erroring is fine in this case. It could throw off the progress display to print lines in this codepath or scare the user on a warning too much.
Someone will probably report a bug that a resource's time elapsed isn't showing up when all of the others are showing up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh thinking on this more I'd be happier with a rework to make the missing times unrepresentable, but that's a minor thing we can pick up when it's convenient.
updated display as per @susanev recommendation |
pkg/backend/display/progress.go
Outdated
if opDuration < 1 { | ||
// Display a more fine-grain duration as the operation | ||
// has completed. | ||
return fmt.Sprintf("%s (%.2f secs)", opText, opDuration) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's nice!
bors merge |
10953: show time elapsed per resource in update r=dixler a=dixler <!--- Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation. --> # Description <!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. --> Fixes #10534 This PR modifies the display to show the time that a resource takes to update. [![asciicast](https://asciinema.org/a/XHYWenULkugoY57kStsadcBH8.svg)](https://asciinema.org/a/XHYWenULkugoY57kStsadcBH8) ## Checklist <!--- Please provide details if the checkbox below is to be left unchecked. --> - [ ] I have added tests that prove my fix is effective or that my feature works. <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Service, then the service should honor older versions of the CLI where this change would not exist. You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add it to the service. --> - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Kyle Dixler <kyle@pulumi.com>
Minor but I obsess over things like this. Instead of “10 secs” should we say “10s”? More standard abbreviation, less real estate, and more consistent with displays elsewhere (including the summary at the end of every update which is alongside this new display so easy to see them at the same time and notice the inconsistency). |
@joeduffy, see #10953 (comment). Kyle already updated the PR to do just that! 😄 |
Build failed: |
bors merge |
10953: show time elapsed per resource in update r=dixler a=dixler <!--- Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation. --> # Description <!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. --> Fixes #10534 This PR modifies the display to show the time that a resource takes to update. [![asciicast](https://asciinema.org/a/XHYWenULkugoY57kStsadcBH8.svg)](https://asciinema.org/a/XHYWenULkugoY57kStsadcBH8) ## Checklist <!--- Please provide details if the checkbox below is to be left unchecked. --> - [ ] I have added tests that prove my fix is effective or that my feature works. <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Service, then the service should honor older versions of the CLI where this change would not exist. You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add it to the service. --> - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Kyle Dixler <kyle@pulumi.com>
Build failed: |
aac11c8
to
e06d901
Compare
bors merge |
10953: show time elapsed per resource in update r=dixler a=dixler <!--- Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation. --> # Description <!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. --> Fixes #10534 This PR modifies the display to show the time that a resource takes to update. [![asciicast](https://asciinema.org/a/XHYWenULkugoY57kStsadcBH8.svg)](https://asciinema.org/a/XHYWenULkugoY57kStsadcBH8) ## Checklist <!--- Please provide details if the checkbox below is to be left unchecked. --> - [ ] I have added tests that prove my fix is effective or that my feature works. <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Service, then the service should honor older versions of the CLI where this change would not exist. You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add it to the service. --> - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Kyle Dixler <kyle@pulumi.com>
Build failed: |
bors merge |
10953: show time elapsed per resource in update r=dixler a=dixler <!--- Thanks so much for your contribution! If this is your first time contributing, please ensure that you have read the [CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md) documentation. --> # Description <!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. --> Fixes #10534 This PR modifies the display to show the time that a resource takes to update. [![asciicast](https://asciinema.org/a/XHYWenULkugoY57kStsadcBH8.svg)](https://asciinema.org/a/XHYWenULkugoY57kStsadcBH8) ## Checklist <!--- Please provide details if the checkbox below is to be left unchecked. --> - [ ] I have added tests that prove my fix is effective or that my feature works. <!--- User-facing changes require a CHANGELOG entry. --> - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change <!-- If the change(s) in this PR is a modification of an existing call to the Pulumi Service, then the service should honor older versions of the CLI where this change would not exist. You must then bump the API version in /pkg/backend/httpstate/client/api.go, as well as add it to the service. --> - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. --> Co-authored-by: Kyle Dixler <kyle@pulumi.com>
Build failed: |
bors retry |
Build succeeded: |
Description
Fixes #10534
This PR modifies the display to show the time that a resource takes to update.
Checklist
make changelog
and committed thechangelog/pending/<file>
documenting my change