-
Notifications
You must be signed in to change notification settings - Fork 250
OTA refactor #912
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
OTA refactor #912
Conversation
9fc745e to
02cf605
Compare
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.
Pull Request Overview
This PR introduces a downgrade/version-specific update capability for JetKVM, allowing users to install specific versions from GitHub releases through a new developer mode feature. The changes refactor the OTA (Over-The-Air) update system into a dedicated internal package with improved state management, and add UI components for version-specific updates with downgrade support.
- Refactored OTA functionality into
internal/otapackage with better separation of concerns - Added support for targeting specific app and system versions for updates/downgrades
- Introduced new UI components and flows for version-specific updates in developer settings
- Enhanced deployment script with connectivity checks and improved error handling
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/ota/*.go |
New OTA package with state management, update logic, and component-based update handling |
ota.go |
Refactored to use new internal OTA package and added RPC methods for component updates |
ui/src/routes/devices.$id.settings.advanced.tsx |
Added developer UI for specifying target versions and initiating version-specific updates |
ui/src/routes/devices.$id.settings.general.update.tsx |
Added downgrade flow and confirmation dialog |
ui/src/components/NestedSettingsGroup.tsx |
New component for visually grouping nested settings |
ui/src/hooks/useVersion.tsx |
Added detection for dev version and downgrade availability fields |
config.go |
Added configurable update API URL support |
scripts/dev_deploy.sh |
Added connectivity checks and SSH options for better deployment reliability |
jsonrpc.go |
Registered new RPC handlers for component-based updates |
Comments suppressed due to low confidence (1)
ui/src/routes/devices.$id.settings.general.update.tsx:18
- Unused variable setSearchParams.
const [searchParams, setSearchParams] = useSearchParams();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
797f705 to
ae197c5
Compare
|
@ym can you add proper handling of 404? Right now, if you specify a version that doesn't exist, you get redirected to the |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…y set in the componentUpdateError function
…multiple languages
…th improved UI components
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Adam Shiervani <adam.shiervani@gmail.com>
| } | ||
|
|
||
| url, err, isCustomVersion := s.getUpdateURL(params) | ||
| traceLogger().Err(err). |
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.
Shouldn't this be inside the if err != nil { block?
|
|
||
| req, err := s.newHTTPRequestWithTrace(ctx, "GET", url, nil, traceLogger) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error creating request: %w", err) |
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.
Should we insert traceLogger().Err(err).Msg("creating request")
internal/ota/ota.go
Outdated
| scopedLogger.Info().Msg("System is up to date") | ||
| } | ||
|
|
||
| s.updating = false |
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.
Should we just wrap this in a defer up higher in the function to ensure we always clear the flag?
|
|
||
| if params.ResetConfig { | ||
| scopedLogger.Info().Msg("Resetting config") | ||
| if err := s.resetConfig(); err != nil { |
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.
Since this resetConfig() call will really run against the current app's code, this means that especially in a downgrade there is potential for us to create a "default" config file that is still not readable by the new app version.
Would it be better to create a sentinel flag that the (new version) app-startup could detect, do the reset config itself, then delete the sentinel? That way we know the default config matches the version loaded.
Of course, we don't already have that code in-place, so downgrading to any current release would not do that work, but it would be worthwhile pushing that out (nowish) as a micro-update just so this version's OTA work would be more robust.
|
|
||
| // We don't need set the updating flag to false here. Either it will; | ||
| // - set to false by the componentUpdateError function | ||
| // - device will reboot |
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.
This is not strictly true, when the if s.rebootNeeded { is not taken, then the flag is still set and we didn't error, nor reboot.
| // fetch the remote metadata | ||
| remoteMetadata, err := s.fetchUpdateMetadata(ctx, params) | ||
| if err != nil { | ||
| if err == ErrVersionNotFound || errors.Unwrap(err) == ErrVersionNotFound { |
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.
if errors.Is(err, ErrVersionNotFound) does the (if needed unwrap) test...
|
I apologize... I never clicked submit on these comments. This issue still concerns me as-merged #912 (comment) |
That's fine, I will open a new PR to resolve these issues. |
Also implements #344
This PR will be backported to https://github.com/jetkvm/kvm/tree/release/049 as part of
0.4.10. Since we don’t plan to introduce i18n in the0.4.xseries, the translation files will be removed during the backport.