feat(bakery): FCOS sysext catalog client for fedora-sysexts/community#720
Open
clubanderson wants to merge 2 commits into
Open
feat(bakery): FCOS sysext catalog client for fedora-sysexts/community#720clubanderson wants to merge 2 commits into
clubanderson wants to merge 2 commits into
Conversation
Implements all sub-scopes from issue projectbluefin#641: - A: FCOS stream metadata client (internal/fcos/streams.go) FetchStreamFedoraVersion fetches the Fedora major version from the FCOS stream JSON at builds.coreos.fedoraproject.org/streams. - B: ParseFCOSTagName (internal/bakery/fcos.go) Left-to-right scan for first '-<digit>' boundary correctly handles names like 'docker-ce', '1password-gui', and versions with epoch fields like 'tailscale-0-1.98.4-1'. - C: FCOSCatalogURL + maxFCOSCatalogPages=20 (internal/bakery/fcos.go) fedora-sysexts/community has ~1600 releases; 20 pages at 100/page provides full coverage with headroom. Emits slog.Warn if truncated. - D: HTTPClient.FetchCatalogFCOS (internal/bakery/fcos.go) Filters releases by arch AND fedoraVersion before deduplication. Fetches SHA256SUMS best-effort. Asset URLs are github.com (confirmed against live API; extensions.fcos.fr CDN is not used). - E: FCOSLookup + fcos_descriptions.go curated catalog Covers tailscale, docker-ce, vscode, vscodium, glab, cilium-cli, 1password-gui, bitwarden with category and TierFCOSCommunity tier. - F: FCOSCatalogClient interface in dispatch.go DispatchingClient.FCOS is FCOSCatalogClient (not bare Client). FetchCatalogForOS gains a fedoraVersion int parameter. Wizard FetchSysexts dispatches via DispatchingClient when available, resolving fedoraVersion lazily from the current stream on first use. FetchFCOSStream is a pre-fetch helper called from main.go startup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dosubot
Bot
added
size:XL
|
Warning Review limit reached
More reviews will be available in 31 minutes and 13 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR implements comprehensive FCOS (Fedora CoreOS) sysext catalog support by adding stream metadata fetching for runtime Fedora version resolution, a full-featured GitHub-backed catalog client with tag parsing and Fedora-version filtering, dispatching infrastructure to route FCOS and Flatcar requests separately, wizard integration with lazy version resolution, and application startup wiring. ChangesFCOS Catalog Client Implementation
Dispatcher and Bakery Integration
Wizard FCOS Integration
Application Startup Wiring
Sequence Diagram(s)sequenceDiagram
participant Wizard as Wizard (main startup)
participant Prefetch as FetchFCOSStream
participant StreamFetch as FetchStreamFedoraVersion
participant StreamsAPI as FCOS Streams API
participant FetchSysexts as FetchSysexts
participant Dispatcher as DispatchingClient
participant FCOSClient as FetchCatalogFCOS
participant GitHubAPI as GitHub Releases API
Wizard->>Prefetch: prefetch FCOS version (best-effort)
Prefetch->>StreamFetch: FetchStreamFedoraVersion(stable)
StreamFetch->>StreamsAPI: GET /stable.json
StreamsAPI-->>StreamFetch: version 44.x.x
StreamFetch-->>Prefetch: 44
Prefetch->>Wizard: cached in State
Wizard->>FetchSysexts: load catalog
FetchSysexts->>Dispatcher: FetchCatalogForOS (FCOS, x86-64, fedoraVersion=44)
Dispatcher->>FCOSClient: FetchCatalogFCOS(x86-64, 44)
FCOSClient->>GitHubAPI: GET /repos/fedora-sysexts/community/releases (paginated)
GitHubAPI-->>FCOSClient: releases (paginated)
FCOSClient->>FCOSClient: filter by arch and Fedora 44
FCOSClient-->>Dispatcher: []SysextEntry
Dispatcher-->>FetchSysexts: filtered catalog
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR adds substantial new functionality across multiple files: FCOS stream version fetching with HTTP handling and JSON parsing; a feature-rich catalog client with pagination, tag parsing, filtering, deduplication, and optional hash fetching; dispatcher interface updates affecting existing code paths; curated metadata catalog; and wizard state integration. High-density implementation logic, diverse file coordination, and multiple edge cases (pagination caps, truncation warnings, floating tags, malformed artifacts, best-effort hash fetching) require careful review across behavior, error handling, and test coverage. Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
internal/bakery/bakery.go (1)
390-392: ⚡ Quick winMake the mock observe
fedoraVersion.This implementation drops the new FCOS parameter, so the dispatch tests still pass even if the caller forwards
0or the wrong stream version. Record the argument onMockClient, or use an FCOS-specific stub in tests that asserts the expected Fedora version.🤖 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/bakery/bakery.go` around lines 390 - 392, MockClient.FetchCatalogFCOS currently ignores the fedoraVersion parameter which lets callers pass wrong versions unnoticed; update FetchCatalogFCOS to record or assert the fedoraVersion before delegating to FetchCatalogArch. Add a field on MockClient (e.g., LastFedoraVersion or ReceivedFedoraVersion) and set it inside FetchCatalogFCOS (or implement an FCOS-specific stub callback on MockClient) so tests can verify the forwarded version, then keep the existing call to m.FetchCatalogArch(ctx, arch). Ensure tests are updated to inspect the new field or use the stub to assert the expected Fedora version.
🤖 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 `@cmd/knuckle/main.go`:
- Around line 154-158: Move the FCOS stream prefetch so it runs before the
sysexts/catalog lazy-resolve: call w.FetchFCOSStream(ctx) prior to invoking
w.FetchSysexts(...) (and before the catalog fetch that triggers lazy resolution
of State.FCOSFedoraVersion). This avoids duplicate stream requests by ensuring
FetchFCOSStream executes as the true prewarm step before Wizard.FetchSysexts can
lazily request the same stream.
In `@internal/bakery/dispatch.go`:
- Around line 39-45: In FetchCatalogForOS (method
DispatchingClient.FetchCatalogForOS) add a validation in the FCOS branch to
reject a missing/zero fedoraVersion: if fedoraVersion == 0 return a descriptive
error (e.g. "fedoraVersion required for FCOS") instead of forwarding 0 into
FetchCatalogFCOS; keep the existing nil-check for d.FCOS and only call
d.FCOS.FetchCatalogFCOS when fedoraVersion is non-zero.
In `@internal/bakery/fcos_descriptions.go`:
- Around line 12-69: fcosCatalog is missing curated entries for "kubernetes" and
"1password-cli", so add two map entries in the fcosCatalog variable (same
structure as existing entries like "tailscale" and "1password-gui"): keys
"kubernetes" and "1password-cli" with fields Category (e.g., "Orchestration" for
kubernetes, "Security" for 1password-cli), SupportTier set to TierFCOSCommunity,
Short and Long descriptive strings, and Caveats (nil or list) as appropriate so
they no longer fall back to raw release text; follow the exact ExtensionMeta
field names used in fcos_descriptions.go.
In `@internal/bakery/fcos_test.go`:
- Around line 131-134: The unit test fixture in internal/bakery/fcos_test.go
includes a SHA256SUMS asset that causes FetchCatalogFCOS to perform an outbound
fetch; update the test to remain hermetic by either removing the fcosAsset entry
with Name "SHA256SUMS" from the Assets slice or changing its BrowserDownloadURL
to point at the httptest server URL used in this test; locate the test data
where Assets: []fcosAsset is defined (the entries with Name "tailscale-..." and
"SHA256SUMS") and modify that SHA256SUMS entry so FetchCatalogFCOS only fetches
from the local test server (or omit the entry entirely) to prevent network calls
during tests.
In `@internal/bakery/fcos.go`:
- Around line 238-245: The code currently permits empty Sha256 when
c.fetchSHA256ForAsset fails (sha256sumsURL, downloadURL, sha256Hash), which
weakens integrity checks; change the logic so that if c.fetchSHA256ForAsset(ctx,
sha256sumsURL, rawFilename) returns an error you do not produce a catalog entry
— either return the error up from the enclosing method or skip the asset and log
a clear error instead of setting sha256Hash="". Additionally implement mandatory
DIGESTS and GPG verification: call a helper that fetches the
.DIGESTS/.DIGESTS.sig and verifies the GPG signature using the embedded Flatcar
key, then verify the asset's SHA512 against the DIGESTS before accepting the
asset (introduce descriptive helper names like fetchAndVerifyDIGESTS(ctx,
digestsURL) and verifySHA512AgainstDIGESTS(rawFilename, digests) and invoke them
in the same code path so failures hard-fail and prevent the entry from being
returned).
In `@internal/wizard/wizard.go`:
- Around line 390-395: The code only fetches FCOSFedoraVersion when it's zero,
so if the user changes w.State.Config.Channel we must invalidate the cached
version; modify the logic around w.State.FCOSFedoraVersion and
fetchFCOSStreamVersionFn to track the channel used for the last fetch (e.g., add
or use a w.State.FCOSChannel field) and if w.State.Config.Channel !=
w.State.FCOSChannel then set w.State.FCOSFedoraVersion = 0 (or otherwise force
refetch) and update w.State.FCOSChannel; ensure this change is applied wherever
the fetch logic runs (the block using model.OSFCOS, w.State.FCOSFedoraVersion,
and fetchFCOSStreamVersionFn) so the Fedora major is re-resolved when the
stream/channel changes.
- Around line 390-397: The code silently ignores failures from
fetchFCOSStreamVersionFn and proceeds to call dc.FetchCatalogForOS with
w.State.FCOSFedoraVersion == 0; change the logic in the block that checks
w.State.Config.OS == model.OSFCOS so that if fetchFCOSStreamVersionFn(ctx,
w.State.Config.Channel) returns a non-nil error (ferr) you propagate that error
(e.g., return or wrap and return the error) instead of continuing, otherwise set
w.State.FCOSFedoraVersion = ver as now; ensure this prevents calling
dc.FetchCatalogForOS with an invalid FCOSFedoraVersion and references the same
symbols (w.State.Config.OS, model.OSFCOS, w.State.FCOSFedoraVersion,
fetchFCOSStreamVersionFn, dc.FetchCatalogForOS).
---
Nitpick comments:
In `@internal/bakery/bakery.go`:
- Around line 390-392: MockClient.FetchCatalogFCOS currently ignores the
fedoraVersion parameter which lets callers pass wrong versions unnoticed; update
FetchCatalogFCOS to record or assert the fedoraVersion before delegating to
FetchCatalogArch. Add a field on MockClient (e.g., LastFedoraVersion or
ReceivedFedoraVersion) and set it inside FetchCatalogFCOS (or implement an
FCOS-specific stub callback on MockClient) so tests can verify the forwarded
version, then keep the existing call to m.FetchCatalogArch(ctx, arch). Ensure
tests are updated to inspect the new field or use the stub to assert the
expected Fedora version.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f7687fe-5a00-4e18-855d-af9bc1f4cb5c
📒 Files selected for processing (10)
cmd/knuckle/main.gointernal/bakery/bakery.gointernal/bakery/dispatch.gointernal/bakery/dispatch_test.gointernal/bakery/fcos.gointernal/bakery/fcos_descriptions.gointernal/bakery/fcos_test.gointernal/fcos/streams.gointernal/fcos/streams_test.gointernal/wizard/wizard.go
- dispatch.go: reject fedoraVersion <= 0 for FCOS (hard error) - wizard.go: propagate stream-version fetch errors; add FCOSFedoraStream for cache invalidation when channel changes - main.go: move FetchFCOSStream before FetchSysexts - fcos.go: hard-fail when SHA256SUMS present but unresolvable - fcos_descriptions.go: add curated entries for kubernetes and 1password-cli - fcos_test.go: remove SHA256SUMS asset from test fixture - bakery.go: add LastFedoraVersion to MockClient; record fedoraVersion param - dispatch_test.go: add test for FCOS fedoraVersion=0 guard Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
2 participants
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.
Closes #641
Summary
Implements all sub-scopes from issue #641 — adds the FCOS sysext catalog client for
fedora-sysexts/community, including stream metadata fetching, tag name parsing, catalog pagination, curated descriptions, and wizard wiring.Changes
A — FCOS stream metadata (
internal/fcos/streams.go)FetchStreamFedoraVersion(ctx, stream)fetches the Fedora major version from the FCOS stream JSON atbuilds.coreos.fedoraproject.org/streams/<stream>.jsonstable,testing, ornext; parses major version fromreleasefieldB — Tag name parsing (
internal/bakery/fcos.go)ParseFCOSTagName(tag)parses<name>-<version>-<fedoraVersion>-<arch>tagsdocker-ce,1password-gui,tailscale-0-1.98.4-1,netbird-ui,cloud-hypervisortailscale,latest,vscode) rejected with descriptive errorC — Catalog URL + pagination (
internal/bakery/fcos.go)FCOSCatalogURLconst;maxFCOSCatalogPages = 20(covers ~1600 releases at 100/page)slog.Warnemitted if cap hit whilerel="next"still exists (soft failure)D —
HTTPClient.FetchCatalogFCOS(internal/bakery/fcos.go)NewFCOSHTTPClient()factorygithub.com(notextensions.fcos.fr)E — Curated descriptions (
internal/bakery/fcos_descriptions.go)FCOSLookup+TierFCOSCommunityconstant** Dispatcher + wizard wiring**F
FCOSCatalogClientinterface indispatch.go(extendsClientwithFetchCatalogFCOS)FetchCatalogForOSgainsfedoraVersion intparameterFetchSysextsdispatches viaDispatchingClient, resolves fedoraVersion lazily on first useWizard.FetchFCOSStreampre-fetch helper called frommain.gostartupmain.gowired toDispatchingClient{Flatcar: NewHTTPClient(), FCOS: NewFCOSHTTPClient()}Tests
internal/fcos/streams_test.go: httptest-based tests for stream version fetchinginternal/bakery/fcos_test.go: table-driven ParseFCOSTagName + FetchCatalogFCOS integration testsgo test ./...pass;go vet ./...andgofmtcleanSummary by CodeRabbit
New Features
Tests