cdn sources for gameap, daemon and gameapctl#22
Merged
Hidden character warning
The head ref may contain hidden characters: "\u0441dn-sources"
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRelease discovery and downloads now use a shared ChangesRelease source migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Action
participant releasesource
participant selector
participant GitHubOrCDN
Action->>releasesource: FindRelease(component, tag, platform)
releasesource->>selector: Resolve release
selector->>GitHubOrCDN: Probe and fetch release metadata
GitHubOrCDN-->>selector: Tag and asset metadata
selector-->>releasesource: Ordered candidate URLs
releasesource-->>Action: Release
Action->>releasesource: Download(release, destination)
releasesource->>GitHubOrCDN: Try candidate URLs
GitHubOrCDN-->>releasesource: Download result
releasesource-->>Action: Completion or error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds a new releasesource package to resolve GameAP component releases from the best available upstream (GitHub Releases API or CDN mirrors), and updates existing install/update flows to use multi-source resolution and download fallback.
Changes:
- Introduces
pkg/releasesourcewith source probing, ordering, failover logic, and download URL candidate generation. - Extends
pkg/releasefinderto support static GitHub-format release lists (for CDN mirrors) and addsNoRetry/IsRateLimitErrorto enable faster failover. - Migrates panel/daemon/selfupdate code paths to use
releasesourcefor release resolution and downloads; adds coverage for new behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/releasesource/source.go | Defines components, source kinds, default source list, and URL builders. |
| pkg/releasesource/selector.go | Implements probing, ordering, and failover logic across sources. |
| pkg/releasesource/releasesource.go | Public API for finding releases and downloading with URL fallback. |
| pkg/releasesource/releasesource_internal_test.go | Adds tests for probing order, failover, rate-limit behavior, and download fallback. |
| pkg/releasefinder/release_finder.go | Adds static-list resolution, NoRetry, IsRateLimitError, and AssetName. |
| pkg/releasefinder/release_finder_test.go | Adds/updates tests for static-list behavior, NoRetry, and AssetName. |
| pkg/panel/install.go | Switches panel install resolution/download to releasesource. |
| internal/actions/selfupdate/self_update.go | Switches self-update release resolution/download to releasesource. |
| internal/actions/panel/update/panel_update_v4.go | Switches panel update resolution/download to releasesource. |
| internal/actions/daemon/update/daemon_update.go | Switches daemon update downloads and resolution to releasesource. |
| internal/actions/daemon/install/daemon_install.go | Switches daemon install downloads and resolution to releasesource. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+104
to
+117
| resp, err := httpClient.Do(req) | ||
| if err != nil { | ||
| return nil, errors.WithMessage(err, "failed to get releases") | ||
| } | ||
|
|
||
| bodyBytes, err := io.ReadAll(resp.Body) | ||
| _ = resp.Body.Close() | ||
| if err != nil { | ||
| return nil, errors.WithMessage(err, "failed to read response body") | ||
| } | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, errors.Errorf("release list request failed with status code %d", resp.StatusCode) | ||
| } |
Coverage Report for CI Build 29057465921Coverage increased (+1.6%) to 13.176%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/actions/daemon/install/daemon_install.go (1)
1429-1447: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate release-resolution helper across daemon install/update.
This function is identical to
findReleaseininternal/actions/daemon/update/daemon_update.go(Lines 266-284): same tag-normalization-for-prerelease logic, samereleasesource.FindRelease(ctx, releasesource.ComponentDaemon, ...)call. If prerelease-detection logic ever changes, it's easy to update one copy and miss the other, causing install/update to diverge in behavior.Consider hoisting this into a shared helper (e.g. in
pkg/releasesource) that both callers invoke.♻️ Proposed shared helper (conceptual, in pkg/releasesource)
// FindReleaseWithPrereleaseDetection resolves a release, automatically // enabling prerelease matching when opts.Tag has a prerelease suffix. func FindReleaseWithPrereleaseDetection( ctx context.Context, component Component, kernel, platform string, opts releasefinder.FindOptions, ) (*Release, error) { if opts.Tag != "" { if norm, normErr := releasefinder.NormalizeTag(opts.Tag); normErr == nil && norm.HasPrereleaseSuffix() { opts.AllowPrerelease = true } } return FindRelease(ctx, component, kernel, platform, opts) }-func findDaemonRelease(ctx context.Context, opts releasefinder.FindOptions) (*releasesource.Release, error) { - if opts.Tag != "" { - if norm, normErr := releasefinder.NormalizeTag(opts.Tag); normErr == nil && norm.HasPrereleaseSuffix() { - opts.AllowPrerelease = true - } - } - - release, err := releasesource.FindRelease( - ctx, - releasesource.ComponentDaemon, - runtime.GOOS, - runtime.GOARCH, - opts, - ) +func findDaemonRelease(ctx context.Context, opts releasefinder.FindOptions) (*releasesource.Release, error) { + release, err := releasesource.FindReleaseWithPrereleaseDetection( + ctx, + releasesource.ComponentDaemon, + runtime.GOOS, + runtime.GOARCH, + opts, + ) if err != nil { return nil, errors.WithMessage(err, "failed to find release") } return release, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/actions/daemon/install/daemon_install.go` around lines 1429 - 1447, Duplicate prerelease-aware release resolution exists in findDaemonRelease and update’s findRelease. Add a shared helper in pkg/releasesource that normalizes opts.Tag, enables AllowPrerelease when appropriate, and delegates to FindRelease; update both callers to use it while preserving their existing error context.pkg/releasesource/selector.go (1)
242-271: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winProbing GitHub burns a request from the very rate-limit budget this feature aims to protect.
probe()sends aHEADrequest to GitHub's API root (https://api.github.com) whenever akindGitHubsource is probed. GitHub counts virtually all REST API requests — HEAD included — against the primary rate limit; onlyGET /rate_limitis exempt. Since probing happens once per process viasync.Once, this costs one unauthenticated-quota request on every invocation just to measure latency/availability, on top of whatever the actual release lookup and any rate-limit retry consume.Consider probing GitHub via
GET /rate_limitinstead (doesn't consume quota) or skipping the network probe for the GitHub source entirely and just relying on the actual find/retry logic to handle unavailability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/releasesource/selector.go` around lines 242 - 271, The probe method sends a quota-consuming HEAD request for GitHub sources. Update selector.probe to avoid probing kindGitHub with the generic request, either by using an exempt GET request to /rate_limit or by returning a non-network probe result and relying on the release lookup/retry logic for GitHub availability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/releasesource/selector.go`:
- Around line 40-92: In selector.findRelease, avoid invoking retryGitHub when a
deterministic content error has already been captured from another source. Guard
the githubRateLimited retry block with contentErr == nil, preserving the
existing retry behavior only when no content error is available, then return
contentErr as currently implemented.
---
Nitpick comments:
In `@internal/actions/daemon/install/daemon_install.go`:
- Around line 1429-1447: Duplicate prerelease-aware release resolution exists in
findDaemonRelease and update’s findRelease. Add a shared helper in
pkg/releasesource that normalizes opts.Tag, enables AllowPrerelease when
appropriate, and delegates to FindRelease; update both callers to use it while
preserving their existing error context.
In `@pkg/releasesource/selector.go`:
- Around line 242-271: The probe method sends a quota-consuming HEAD request for
GitHub sources. Update selector.probe to avoid probing kindGitHub with the
generic request, either by using an exempt GET request to /rate_limit or by
returning a non-network probe result and relying on the release lookup/retry
logic for GitHub availability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28ef0e96-bda7-4e5d-8053-c59456fedb5d
📒 Files selected for processing (11)
internal/actions/daemon/install/daemon_install.gointernal/actions/daemon/update/daemon_update.gointernal/actions/panel/update/panel_update_v4.gointernal/actions/selfupdate/self_update.gopkg/panel/install.gopkg/releasefinder/release_finder.gopkg/releasefinder/release_finder_test.gopkg/releasesource/releasesource.gopkg/releasesource/releasesource_internal_test.gopkg/releasesource/selector.gopkg/releasesource/source.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes