fix(dockers/v2): check docker driver on healthcheck #6273
Conversation
Deploying goreleaser with
|
| Latest commit: |
bb641be
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://829db0d0.goreleaser.pages.dev |
| Branch Preview URL: | https://hc-docker-buildx.goreleaser.pages.dev |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR separates prerequisite verification from runtime checks by introducing public DependencyChecker and HealthChecker interfaces and global slices DependencyCheckers and HealthCheckers. The healthcheck command first iterates DependencyCheckers to verify required tools, then iterates HealthCheckers calling Healthcheck(ctx) and logs per-check outcomes; errors are aggregated and return the single message "one or more checks failed" on failure. Docker v2 Base now implements Healthcheck to inspect and validate the buildx driver via getBuildxDriver and isDriverValid. Tests were updated and new tests assert the checker slices and driver validator. Sequence Diagram(s)sequenceDiagram
participant CLI as goreleaser healthcheck cmd
participant Manager as pkg/healthcheck (manager)
participant Dep as DependencyChecker (various)
participant HCk as HealthChecker (various)
participant Docker as internal/pipe/docker/v2/Base
CLI->>Manager: Run healthcheck(ctx)
Manager->>Dep: Iterate DependencyCheckers
loop per DependencyChecker
Dep->>Dep: Dependencies(ctx) / verify tools
Dep-->>Manager: ok / error (logged)
end
Manager-->>CLI: dependency results collected
CLI->>Manager: proceed to runtime health checks
Manager->>HCk: Iterate HealthCheckers
loop per HealthChecker
HCk->>HCk: Healthcheck(ctx)
alt HealthChecker is Docker v2 Base
HCk->>Docker: Healthcheck(ctx)
Docker->>Docker: getBuildxDriver -> isDriverValid
Docker-->>HCk: valid / invalid / error (logged)
end
HCk-->>Manager: ok / error (logged)
end
Manager-->>CLI: aggregate errors -> "one or more checks failed" | success
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used🧬 Code graph analysis (3)pkg/healthcheck/healthcheck.go (2)
internal/pipe/docker/v2/docker_integration_test.go (3)
internal/pipe/docker/v2/docker.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (10)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/healthcheck.go(2 hunks)internal/pipe/docker/v2/docker.go(4 hunks)pkg/healthcheck/healthcheck.go(1 hunks)pkg/healthcheck/healthcheck_test.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/healthcheck.go (2)
pkg/healthcheck/healthcheck.go (2)
DependencyCheckers(46-59)HealthCheckers(39-41)internal/middleware/skip/skip.go (1)
Maybe(28-46)
pkg/healthcheck/healthcheck.go (4)
pkg/context/context.go (1)
Context(80-115)internal/pipe/winget/template.go (1)
Dependencies(100-102)pkg/build/build.go (1)
Dependencies(36-46)internal/pipe/docker/v2/docker.go (5)
Base(36-36)Base(48-48)Base(51-51)Base(54-56)Base(62-87)
internal/pipe/docker/v2/docker.go (2)
pkg/context/context.go (1)
Context(80-115)internal/logext/styles.go (3)
Warning(18-21)Keyword(24-27)URL(13-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: check
- GitHub Check: ruleguard / scan
- GitHub Check: test (windows-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: analyze (go)
- GitHub Check: lint
- GitHub Check: Cloudflare Pages
🔇 Additional comments (16)
internal/pipe/docker/v2/docker.go (5)
47-48: LGTM!The
Dependenciesmethod correctly implements theDependencyCheckerinterface and returns the requireddocker buildxdependency.
50-51: LGTM!The
Healthcheckmethod correctly implements theHealthCheckerinterface and delegates tocheckBuildxDriverto verify buildx installation.
92-92: LGTM!Explicitly ignoring the error here is appropriate since the healthcheck command will catch buildx issues early, while actual snapshot builds should proceed with warnings rather than failing.
113-113: LGTM!Explicitly ignoring the error here is appropriate since the healthcheck command will catch buildx issues early, while actual publish operations should proceed with warnings rather than failing.
517-535: LGTM!The refactored
checkBuildxDrivercorrectly returns an error for unknown drivers while logging a warning. The dual behavior (logging + returning error) allows the healthcheck command to fail early while letting actual build/publish operations continue with warnings.pkg/healthcheck/healthcheck_test.go (4)
19-19: LGTM!The update to use
testctx.Wrap(t.Context())aligns with the new test context wrapping pattern introduced in the codebase.
28-36: LGTM!The update to use
testctx.WrapWithCfg(t.Context(), config.Project{...})aligns with the new test context wrapping pattern.
53-55: LGTM!The test validates that the
HealthCheckersslice is properly initialized, providing basic coverage for the new healthcheck infrastructure.
57-59: LGTM!The test validates that the
DependencyCheckersslice is properly initialized, providing basic coverage for the dependency checking infrastructure.cmd/healthcheck.go (3)
58-67: LGTM!The first phase correctly iterates over
DependencyCheckers, respecting skip conditions and validating that required tools are present in the PATH.
68-75: LGTM!The second phase correctly iterates over
HealthCheckers, respecting skip conditions and running complex healthchecks beyond simple PATH validation.
82-82: LGTM!The updated error message "healthcheck failed" is more appropriate since it now covers both dependency checks and complex healthchecks, not just missing tools.
pkg/healthcheck/healthcheck.go (4)
19-26: LGTM!The
HealthCheckerinterface is well-designed, providing a clear contract for complex health checks that go beyond simple dependency validation.
28-34: LGTM!The
DependencyCheckerinterface is well-defined and provides a clear contract for dependency validation.
36-41: LGTM!The
HealthCheckersslice is properly initialized withdockerv2.Base{}, which implements the buildx driver check as intended by the PR objectives.
46-59: LGTM!The
DependencyCheckersslice is properly initialized with all relevant dependency checkers. Note thatdockerv2.Base{}correctly appears in bothHealthCheckersandDependencyCheckerssince it implements both interfaces.
9d1f09f to
c458b47
Compare
closes #6137 Signed-off-by: Carlos Alexandro Becker <[email protected]>
c458b47 to
d285422
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/healthcheck/healthcheck.go (1)
43-43: Typo still present.The past review comment indicated this was addressed in commit c458b47, but the double period ("checkers..") still appears in the annotated code. Please verify the fix was actually applied.
#!/bin/bash # Verify if the typo was fixed in the actual file grep -n "DependencyCheckers is the list" pkg/healthcheck/healthcheck.go
🧹 Nitpick comments (1)
cmd/healthcheck.go (1)
97-106: Consider extracting the shared success style.The green checkmark style is duplicated between
check()(line 99) andcheckPath()(line 125). Consider extracting to a package-level variable for consistency and minor efficiency.var toolsChecked = &sync.Map{} +var successStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("2")).Bold(true) func check(name string, err error) error { if err == nil { - st := lipgloss.NewStyle().Foreground(lipgloss.Color("2")).Bold(true) - log.Infof("%s %s", st.Render("✓"), codeStyle.Render(name)) + log.Infof("%s %s", successStyle.Render("✓"), codeStyle.Render(name)) return nil }And similarly update
checkPath()at line 125-126.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/healthcheck.go(3 hunks)internal/pipe/docker/v2/docker.go(3 hunks)internal/pipe/docker/v2/docker_test.go(1 hunks)pkg/healthcheck/healthcheck.go(1 hunks)pkg/healthcheck/healthcheck_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/pipe/docker/v2/docker.go
- internal/pipe/docker/v2/docker_test.go
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/healthcheck.go (2)
pkg/healthcheck/healthcheck.go (2)
DependencyCheckers(46-59)HealthCheckers(39-41)internal/middleware/skip/skip.go (1)
Maybe(28-46)
pkg/healthcheck/healthcheck.go (1)
internal/pipe/docker/v2/docker.go (5)
Base(36-36)Base(48-48)Base(51-57)Base(60-62)Base(68-93)
pkg/healthcheck/healthcheck_test.go (4)
pkg/context/context.go (1)
Context(80-115)internal/testctx/testctx.go (1)
WrapWithCfg(126-132)pkg/config/config.go (1)
Project(1194-1262)pkg/healthcheck/healthcheck.go (2)
HealthCheckers(39-41)DependencyCheckers(46-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: ruleguard / scan
- GitHub Check: test (ubuntu-latest)
- GitHub Check: check
- GitHub Check: test (windows-latest)
- GitHub Check: lint
- GitHub Check: analyze (go)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (8)
pkg/healthcheck/healthcheck_test.go (3)
18-21: LGTM!The switch to
testctx.Wrap(t.Context())properly propagates the test context, which is the idiomatic approach for Go 1.24+ test context handling.
27-36: LGTM!Using
testctx.WrapWithCfg(t.Context(), config.Project{...})correctly wraps the test context with configuration, maintaining consistency with the pattern used throughout the test file.
53-59: LGTM!These guard-rail tests ensure the global
HealthCheckersandDependencyCheckersslices are properly initialized. While simple, they catch accidental removal of entries during refactoring.cmd/healthcheck.go (2)
58-75: LGTM!The separation of
DependencyCheckers(binary path verification) andHealthCheckers(complex validation like buildx driver check) is a clean design. Both loops correctly useskip.Maybeto respect the skipper interface and aggregate errors appropriately.
82-82: LGTM!The updated error message "one or more checks failed" is more generic and accurately reflects that both dependency checks and health checks are now performed.
pkg/healthcheck/healthcheck.go (3)
19-26: LGTM!The
HealthCheckerinterface is well-designed for complex validations that go beyond simple binary existence checks. The documentation clearly distinguishes its purpose fromDependencyChecker.
28-34: LGTM!The
DependencyCheckerinterface cleanly separates binary path verification from more complex health checks.
36-41: Good design:dockerv2.Base{}in both slices is intentional.The dual registration makes sense:
DependencyCheckerverifiesdocker buildxbinary exists in PATH, whileHealthCheckervalidates the buildx driver configuration. This layered approach catches both missing binaries and misconfigured drivers.Also applies to: 46-59
Signed-off-by: Carlos Alexandro Becker <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR refactors the healthcheck system to separate simple dependency checks (binary presence in PATH) from complex health checks (validation logic like docker driver verification). It specifically adds a healthcheck for the docker v2 pipe to validate that the docker buildx driver is set to docker-container, addressing issue #6137.
Key Changes
- Split
Healthcheckerinterface into two distinct interfaces:DependencyChecker(for binary PATH checks) andHealthChecker(for complex validation) - Added
Healthcheckmethod to docker v2 pipe to validate buildx driver configuration - Updated test files to use the new
testctx.Wrap()pattern instead of deprecatedtestctx.New()
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/healthcheck/healthcheck.go | Splits the single Healthchecker interface into HealthChecker and DependencyChecker, creates separate lists for each type |
| internal/pipe/docker/v2/docker.go | Adds Healthcheck method to validate docker buildx driver, refactors isDriverValid and error handling |
| cmd/healthcheck.go | Updates to iterate over both HealthCheckers and DependencyCheckers separately, adds check function for health validation |
| pkg/healthcheck/healthcheck_test.go | Updates test context creation to use testctx.Wrap(), adds basic tests for new exported lists |
| internal/pipe/docker/v2/docker_test.go | Adds test for isDriverValid function, updates test context creation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Carlos Alexandro Becker <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Carlos Alexandro Becker <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6273 +/- ##
==========================================
- Coverage 85.56% 85.48% -0.09%
==========================================
Files 170 170
Lines 14379 14401 +22
==========================================
+ Hits 12304 12311 +7
- Misses 1437 1451 +14
- Partials 638 639 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
closes #6137