Only warn about version upgrade once a day #12660
Conversation
Changelog[uncommitted] (2023-06-13)Features
|
pkg/cmd/pulumi/pulumi.go
Outdated
| var skipUpdateCheck bool | ||
| latestVer, oldestAllowedVer, err := getCachedVersionInfo() | ||
| if err == nil { | ||
| // If we have cached version information then don't bother warning, we would have warned when we last | ||
| // updated the cache. But we will still log this to the internal logging system that by default users | ||
| // don't see. | ||
| skipUpdateCheck = true | ||
| } else { |
There was a problem hiding this comment.
nit: This could be an early exit and simplify the branching here and in the remainder of the function which is dependent on mutable state.
There was a problem hiding this comment.
+1 to early return here.
Long-term, it appears that with this change the cache file is useless as a source of information, and is just a marker. We can (here or in a future change) stop storing the JSON-encoded blob in it, and instead just use its last modified time to determine whether we've already informed the user.
There was a problem hiding this comment.
+1 to early return here.
Can't early return because I still wanted to do the version check and write to the logging log.
Long-term, it appears that with this change the cache file is useless as a source of information, and is just a marker.
Yes, I did consider changing that all with this change but wasn't sure if anything else might use this file.
There was a problem hiding this comment.
it's a nit so no need to block on this.
pkg/cmd/pulumi/pulumi.go
Outdated
| var skipUpdateCheck bool | ||
| latestVer, oldestAllowedVer, err := getCachedVersionInfo() | ||
| if err == nil { | ||
| // If we have cached version information then don't bother warning, we would have warned when we last | ||
| // updated the cache. But we will still log this to the internal logging system that by default users | ||
| // don't see. | ||
| skipUpdateCheck = true | ||
| } else { |
There was a problem hiding this comment.
+1 to early return here.
Long-term, it appears that with this change the cache file is useless as a source of information, and is just a marker. We can (here or in a future change) stop storing the JSON-encoded blob in it, and instead just use its last modified time to determine whether we've already informed the user.
Ugh maybe, but this code is not built for testing 😆 |
Yeah, I think without refactoring it, it's possible with a little |
pkg/cmd/pulumi/pulumi.go
Outdated
| var skipUpdateCheck bool | ||
| latestVer, oldestAllowedVer, err := getCachedVersionInfo() | ||
| if err == nil { | ||
| // If we have cached version information then don't bother warning, we would have warned when we last | ||
| // updated the cache. But we will still log this to the internal logging system that by default users | ||
| // don't see. | ||
| skipUpdateCheck = true | ||
| } else { |
There was a problem hiding this comment.
it's a nit so no need to block on this.
Changes the new CLI version check to warn about new versions at most once a day. Fixes #12659
Adds a test to verify that checkForUpdate does not warn more than once in the same 24 hour period.
abhinav
force-pushed
the
fraser/versionWarning
branch
from
71c0ce2
to
5164286
Compare
|
@Frassle @dixler I rebased this and added a test to verify this new behavior. The test fails without this change: |
|
bors merge |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page.
|
Description
Fixes #12659.
Checklist
make changelogand committed thechangelog/pending/<file>documenting my change