[CFX-5210] Implement plugin update check functionality and state management#402
[CFX-5210] Implement plugin update check functionality and state management#402
Conversation
feat: add state management for the version checks and cooldown for the updates
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| DisableSuggestions: true, | ||
| Run: func(_ *cobra.Command, args []string) { | ||
| checkAndPromptPluginUpdate(pluginName, manifest.Version, pluginPath) | ||
|
|
There was a problem hiding this comment.
Stale manifest used after successful in-place plugin update
Medium Severity
After checkAndPromptPluginUpdate successfully updates the plugin on disk, ExecutePlugin is called with the manifest struct captured at command-registration time. This stale manifest may have an outdated Authentication field, causing the auth check to be skipped (or unnecessarily enforced) for the newly installed version. The Version in the user-agent header will also be wrong. The issue only affects the first execution immediately after the in-place update.
Additional Locations (1)
|
|
||
| const ( | ||
| // DefaultUpdateCheckInterval is the default cooldown between update checks for a given plugin. | ||
| DefaultUpdateCheckInterval = 24 * time.Hour |
There was a problem hiding this comment.
Maybe we should stick to a lower value? We sometimes release twice a day... So maybe 1 hour... What's the downside of a lower value?
There was a problem hiding this comment.
I don't think there's a downside. Given this is configurable.
victorborshak
left a comment
victorborshak
left a comment
There was a problem hiding this comment.
LGTM. Left a comment about using 1 hour instead of 24 hour cooldown
ajalon1
left a comment
ajalon1
left a comment
There was a problem hiding this comment.
@cdevent Requesting changes so that you can make sure to update docs/. You can do it in this PR or the next, but please do so since this is a large bit of functionality.
Still reviewing.
Also, agree w. Victor let's set it to one hour. That still feels slow enough.
ajalon1
left a comment
ajalon1
left a comment
There was a problem hiding this comment.
Leaving Request Changes in place for these reasons:
- update the docs for the new global state.yaml
- update the docs to document new configs
- consider defer state.SetLastPluginCheck() in root
- change DefaultUpdateCheckInterval to ~1 hour like @victorborshak suggested
Any other changes or suggestions I'm fine with handling later
| "gopkg.in/yaml.v3" | ||
| ) | ||
|
|
||
| const globalStateFileName = "state.yaml" |
There was a problem hiding this comment.
😁 If we haven't already, we should standardize on .yaml ;)
| if !askYesNo() { | ||
| // User declined — reset cooldown so they are not nagged again | ||
| state.SetLastPluginCheck(pluginName) | ||
| fmt.Println() | ||
|
|
||
| return | ||
| } |
There was a problem hiding this comment.
I would suggest a log.Debug() here.
There was a problem hiding this comment.
I think this file is getting large enough that we might pull plugin code into a separate module. Not a blocker.
| if viper.GetBool("skip-plugin-update-check") { | ||
| return | ||
| } |
There was a problem hiding this comment.
Thanks for checking from viper ;)
| // Only check managed plugins (those under ~/.config/datarobot/plugins/) | ||
| if !isManagedPlugin(pluginPath) { | ||
| return | ||
| } | ||
|
|
||
| result := internalPlugin.CheckForUpdate(pluginName, installedVersion, internalPlugin.PluginRegistryURL) | ||
| if result == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
This in particularly feels like it could go under plugin/shared/helpers. But maybe it's fine here for now.
| if !isManagedPlugin(pluginPath) { | ||
| return | ||
| } |
There was a problem hiding this comment.
I would suggest a log.Debug() here.
|
|
||
| if !askYesNo() { | ||
| // User declined — reset cooldown so they are not nagged again | ||
| state.SetLastPluginCheck(pluginName) |
There was a problem hiding this comment.
I think we can defer state.SetLastPluginCheck(pluginName) earlier, say before we call internalPlugin.CheckForUpdate.
That way we don't have to invoke it twice here on line 333 and on line 342.
4 participants
RATIONALE
This pull request introduces a new automatic plugin update check feature for managed plugins, along with user prompts and cooldowns to prevent repeated notifications. It also adds configuration options for controlling the update check interval and enables robust error handling and rollback for plugin updates. The changes are grouped into new functionality, configuration improvements, and test coverage.
CHANGES
New plugin update check functionality:
internal/plugin/updatecheck.gomodule implementing plugin update checks, cooldown logic, version comparison, and registry fetch with timeout. This checks for updates before running managed plugins and prompts the user to upgrade if a newer version is available.cmd/root.go, including user interaction, backup, install, validation, and rollback on failure. Cooldown is reset regardless of user action or update outcome. [1] [2] [3]Configuration and usability improvements:
plugin-update-check-interval(controls cooldown between update checks) andskip-plugin-update-check(disables pre-run checks), with sensible defaults and documentation. [1] [2]Test coverage and reliability:
internal/plugin/updatecheck_test.gocovering all cases: disabled checks, cooldowns, network errors, version comparisons (including non-semver), registry errors, and successful update detection.These changes collectively make plugin management more user-friendly and robust, reducing unnecessary prompts and handling failures gracefully.
PR Automation
Comment-Commands: Trigger CI by commenting on the PR:
/trigger-smoke-testor/trigger-test-smoke- Run smoke tests/trigger-install-testor/trigger-test-install- Run installation testsLabels: Apply labels to trigger workflows:
run-smoke-testsorgo- Run smoke tests on demand (only works for non-forked PRs)Important
For Forked PRs: If you're an external contributor, the
run-smoke-testslabel won't work. Only maintainers can trigger smoke tests on forked PRs by applying theapproved-for-smoke-testslabel after security review. Please comment requesting maintainer review if you need smoke tests to run.Note
Medium Risk
Adds new pre-execution network-based update checks and an interactive upgrade prompt for managed plugins, plus new persisted global state; failures could affect plugin startup UX and update behavior despite rollback safeguards.
Overview
Adds an automatic managed-plugin update check that runs before executing discovered plugins, compares installed vs latest registry version, and prompts the user to upgrade (with
--skip-plugin-update-checkand configurable--plugin-update-check-interval).Introduces a new user-global state file (
~/.config/datarobot/state.yaml) to persist per-plugin cooldown timestamps, and refactors registry fetching to support context/timeouts for fast, non-blocking checks.Refactors the manual
dr plugin updateflow to reuse a sharedRunPluginUpdate(backup → install → validate → rollback) implementation, adds a reusable stdinAskYesNohelper, and includes new unit tests for update checking and global state persistence.Written by Cursor Bugbot for commit db0569a. This will update automatically on new commits. Configure here.