feat: Add support for world cardinal build
#113
Conversation
- just builds the cardinal docker image, doesn't run anything
How to use the Graphite Merge QueueAdd the label graphite/merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
WalkthroughThis update introduces a new CLI command in the Cardinal package for building a game image using the Cobra library. The changes integrate configuration management, concurrent processing for Docker image building, and optional image pushing. Additionally, the Docker client gains new methods to manage build and push operations, and the Cardinal service’s Docker dependency version is upgraded. A custom function for creating BubbleTea programs is added and integrated into the region selector flow within forge test and project logic, with updated error handling and improved test assertions. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User CLI
participant CMD as buildCmd
participant Client as Docker Client
participant Proc as Build Process
User->>CMD: Invoke "build" command
CMD->>CMD: Retrieve configuration & flags
CMD->>Client: Call Build(ctx, pushTo, pushAuth, serviceBuilders...)
Client->>Client: Process volume creation
Client->>Proc: Pull images for services
Proc-->>Client: Images pulled
Client->>Proc: Build images concurrently
Proc-->>Client: Build complete
alt Push option enabled
Client->>Client: Execute pushImages
else
Client->>Client: Skip push step
end
Client-->>CMD: Return success or error
CMD-->>User: Output build status
sequenceDiagram
participant Caller as Forge Code/Test
participant NTP as NewTeaProgram
participant BTea as BubbleTea Program
Caller->>NTP: Instantiate region selector
NTP->>NTP: Check if stderr is a TTY
NTP->>BTea: Configure program options (use std input if not TTY)
BTea-->>Caller: Return program instance
Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
![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)
common/docker/client.go (1)
72-107: Good implementation of the Build method.The method correctly implements the functionality to build the Cardinal Docker image without starting containers. It properly handles volume creation, service instantiation, image pulling, and conditional building.
There are commented out lines related to network creation (76-79) that could be removed since they're not needed for the build functionality.
func (c *Client) Build(ctx context.Context, serviceBuilders ...service.Builder) error { namespace := c.cfg.DockerEnv["CARDINAL_NAMESPACE"] - //err := c.createNetworkIfNotExists(ctx, namespace) - //if err != nil { - // return eris.Wrap(err, "Failed to create network") - //} err := c.processVolume(ctx, CREATE, namespace)cmd/world/cardinal/build.go (1)
41-65: Consider consolidating log level validation.The code has multiple blocks handling log level validation and defaults. Consider refactoring this into a helper function to improve readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/world/cardinal/build.go(1 hunks)cmd/world/cardinal/cardinal.go(2 hunks)common/docker/client.go(1 hunks)common/docker/service/cardinal.go(1 hunks)
🔇 Additional comments (6)
cmd/world/cardinal/cardinal.go (2)
54-57: Good implementation of the Cardinal-specific service retriever.This helper function properly isolates the Cardinal service from other services, which aligns well with the PR objective of building only the Cardinal Docker image.
39-40: LGTM - Command registration looks good.The build command is properly integrated into the existing command structure and flag registration system.
common/docker/service/cardinal.go (1)
114-117: Good update to Go 1.24.Upgrading from Go 1.22 to Go 1.24 is a positive change as it includes performance improvements, security enhancements, and bug fixes that will benefit the Cardinal service.
cmd/world/cardinal/build.go (3)
22-103: Well-structured command implementation.The build command is well-implemented with appropriate error handling, configuration management, and user feedback. It follows the established pattern for Cobra commands in this codebase.
89-94: Verify error handling for successful build.On line 94, you're returning a wrapped error
ErrGracefulExitwhen the build completes successfully. This seems unusual since this is the success path. Consider returningnilinstead of an error on successful completion.group.Go(func() error { if err := dockerClient.Build(ctx, services...); err != nil { return eris.Wrap(err, "Encountered an error with Docker") } - return eris.Wrap(ErrGracefulExit, "Stack terminated") + return nil })
1-15:✅ Verification successful
Ensure all used variables are defined.
The code uses several variables that aren't defined in this file:
DockerCardinalEnvLogLevelflagLogLevelvalidLogLevelsCardinalPortErrGracefulExitThese are likely defined in another file in the package but should be verified.
🏁 Script executed:
#!/bin/bash # Description: Verify the existence of undefined variables used in build.go echo "Checking for variable definitions in the cardinal package..." rg -A 1 "DockerCardinalEnvLogLevel|flagLogLevel|validLogLevels|CardinalPort|ErrGracefulExit" --type go cmd/world/cardinal/Length of output: 5301
Confirmed Definitions in the Cardinal Package
All the variables referenced in
build.gohave been verified to be defined within the package:
DockerCardinalEnvLogLevel,flagLogLevel, andvalidLogLevelsare defined incmd/world/cardinal/start.go.CardinalPortis defined incmd/world/cardinal/dev.go.ErrGracefulExitis defined incmd/world/cardinal/start.go.No further action is required regarding variable definitions.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
==========================================
- Coverage 49.81% 48.15% -1.66%
==========================================
Files 64 65 +1
Lines 5785 6030 +245
==========================================
+ Hits 2882 2904 +22
- Misses 2495 2714 +219
- Partials 408 412 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
common/docker/client.go (1)
72-100: New Build method looks good, with a suggestion for code reuseThe implementation of the new
Buildmethod is well-structured and follows the project's existing patterns for Docker operations. It properly creates volumes, pulls images, and builds Docker images without starting containers.I noticed that the code for creating service instances from builders (lines 82-86) is duplicated across several methods in this file (Build, Start, Stop, Purge). Consider extracting this common functionality into a helper method to reduce duplication and improve maintainability.
+func (c *Client) createServices(serviceBuilders ...service.Builder) []service.Service { + dockerServices := make([]service.Service, 0) + for _, sb := range serviceBuilders { + ds := sb(c.cfg) + dockerServices = append(dockerServices, ds) + } + return dockerServices +} func (c *Client) Build(ctx context.Context, serviceBuilders ...service.Builder) error { namespace := c.cfg.DockerEnv["CARDINAL_NAMESPACE"] err := c.processVolume(ctx, CREATE, namespace) if err != nil { return eris.Wrap(err, "Failed to create volume") } - // get all services - dockerServices := make([]service.Service, 0) - for _, sb := range serviceBuilders { - ds := sb(c.cfg) - dockerServices = append(dockerServices, ds) - } + // get all services + dockerServices := c.createServices(serviceBuilders...)The method correctly implements the PR objective to add support for building the Cardinal Docker image without executing additional processes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
cmd/world/cardinal/cardinal.go(2 hunks)common/docker/client.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/world/cardinal/cardinal.go
- new flag push to indicate a push should be done - new flags used by push: auth, user, pass, and regtoken; all used for authentication - added NewTeaProgram() to work without TTY under debugger - workaround for bug in tests were region selector was sometimes nil - switch to direct tea program
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
cmd/world/root/create.go (1)
209-209: Confirm consistency offorge.NewTeaProgram.
This change aligns with the shift fromtea.NewProgramto the customforgeimplementation. Ensure that other references to the Bubble Tea program in this package are consistently updated.cmd/world/forge/forge_test.go (3)
1590-1590: Consider splitting this test to avoid high cognitive complexity.
The//nolint:gocognitannotation suppresses complexity warnings. Overly large or deeply nested tests can be brittle and harder to maintain.
1741-1749: Handlenilregion selector with explicit test failure.
Instead of only printing the message"failed to create region selector", consider failing the test (e.g., usings.Require().NotNil(regionSelector)) to avoid masking potential errors.
1758-1764: Use assertions to handle anilregion selector in tests.
Relying only onprint("region selector is nil")can make it harder to detect errors in CI. Switching to an assertion (e.g.,s.Require().NotNil) or returning early ensures quick feedback.common/docker/client.go (1)
72-109: Well implemented Docker image build functionality.The new
Buildmethod provides a streamlined approach to building Cardinal Docker images without starting containers. The implementation follows similar patterns to other client methods with proper error handling and a clear workflow.Consider adding a parallel function to the existing
Startmethod's deferred cleanup. While volumes are created, there's no cleanup mechanism if the build succeeds but subsequent operations fail. This would be especially helpful in CI/CD environments where cleanup is important.common/docker/client_image.go (2)
487-602: Well-structured image push implementation.The new
pushImagesmethod is well-implemented with proper progress tracking, error handling, and concurrent operations. It follows patterns established in thepullImagesmethod for consistency.Consider enhancing error handling in the image inspection section (lines 505-508). Currently, it returns early on the first error. A more robust approach might be to collect these errors and continue with valid images, especially if pushing multiple images:
- _, _, err := c.client.ImageInspectWithRaw(ctx, imageName) - if err != nil { - return eris.New(fmt.Sprintf("Error inspecting image %s for service %s: %v\n", - imageName, service.Name, err)) - } + _, _, err := c.client.ImageInspectWithRaw(ctx, imageName) + if err != nil { + errChan <- eris.New(fmt.Sprintf("Error inspecting image %s for service %s: %v\n", + imageName, service.Name, err)) + wg.Done() // Reduce wait counter for this image + continue // Skip this image but continue with others + }
424-424: Minor: Code duplication in decoder loop.The comment
//nolint:dupl // different commandsaddresses the duplication between this loop and the similar one inpushImages, but consider refactoring this into a shared helper function to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (11)
cmd/world/cardinal/build.go(1 hunks)cmd/world/forge/common.go(2 hunks)cmd/world/forge/forge_test.go(3 hunks)cmd/world/forge/project.go(1 hunks)cmd/world/root/create.go(2 hunks)cmd/world/root/doctor.go(2 hunks)common/docker/client.go(1 hunks)common/docker/client_container.go(2 hunks)common/docker/client_image.go(4 hunks)common/docker/client_network.go(1 hunks)common/docker/client_volume.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cmd/world/forge/project.go
🧰 Additional context used
🧬 Code Definitions (5)
common/docker/client_volume.go (5)
common/docker/client_container.go (8) (8)
c(26-113)c(115-135)c(137-147)c(149-165)c(167-187)c(189-214)c(216-282)processType(24-24)common/docker/client_network.go (1) (1)
c(14-89)common/docker/client_image.go (1) (1)
c(31-123)cmd/world/forge/common.go (1) (1)
NewTeaProgram(251-257)tea/component/multispinner/multispinner.go (1) (1)
CreateSpinner(38-62)
common/docker/client.go (2)
common/docker/client_volume.go (1) (1)
c(15-100)common/docker/client_image.go (8) (8)
c(31-123)c(125-167)c(172-220)c(231-284)c(286-316)c(318-337)c(343-369)err(241-241)
common/docker/client_image.go (2)
cmd/world/forge/common.go (2) (2)
NewTeaProgram(251-257)err(40-40)tea/component/multispinner/multispinner.go (1) (1)
CreateSpinner(38-62)
common/docker/client_container.go (2)
cmd/world/forge/common.go (1) (1)
NewTeaProgram(251-257)tea/component/multispinner/multispinner.go (1) (1)
CreateSpinner(38-62)
cmd/world/forge/forge_test.go (2)
cmd/world/forge/project.go (1) (1)
regionSelector(21-21)cmd/world/forge/common.go (1) (1)
NewTeaProgram(251-257)
🔇 Additional comments (15)
cmd/world/forge/common.go (1)
249-257: Good implementation of theNewTeaProgramutility function.This function provides a consistent way to handle TTY detection when creating BubbleTea programs. It's a useful abstraction that ensures programs work correctly both in terminal and non-terminal environments (like debuggers).
The commented-out line suggests you considered removing the renderer altogether, but decided to keep it. This maintains visual output while still fixing input handling for non-TTY environments.
common/docker/client_network.go (2)
9-9: Updated import to use the forge package.The change correctly imports the forge package containing the new NewTeaProgram utility function.
17-17: Consistent use of the new NewTeaProgram utility.This change ensures that the network creation process properly handles both TTY and non-TTY environments, aligning with the standardization approach across the codebase.
cmd/world/root/doctor.go (2)
9-9: Added forge package import.The change correctly imports the forge package containing the NewTeaProgram utility function.
87-87: Standardized creation of BubbleTea programs.The change from direct
tea.NewProgramtoforge.NewTeaProgramensures consistent handling of terminal environments across the codebase, particularly important for debugging scenarios.common/docker/client_volume.go (2)
10-10: Updated import to use the forge package.The change correctly imports the forge package containing the NewTeaProgram utility function.
18-18: Standardized creation of BubbleTea programs.The change from direct
tea.NewProgramtoforge.NewTeaProgramensures consistent handling of terminal environments across the codebase. This is particularly important for volume operations that need to work reliably in both interactive and automated contexts.cmd/world/root/create.go (1)
11-11: Importing theforgepackage looks consistent.
No issues detected with the added import statement.cmd/world/cardinal/build.go (5)
1-15: Package and imports are well-structured.
The chosen imports for Docker registry, error handling, logging, and concurrency appear consistent with the file’s functionality.
17-23: Flag constants named clearly.
The constants for push and authentication are self-explanatory, improving the command’s clarity.
29-31: Command usage description is concise.
The short and long descriptions accurately convey the purpose of thebuildcommand.
32-127: Ensure correct build flag usage for Docker.
From earlier feedback, the Docker build operation may requirecfg.Build = true. Revisit whether this is still necessary here.+ // Ensure Build flag is enabled before building + cfg.Build = true
130-137: Flag registrations look appropriate.
Registering build-related flags for log level, debugging, telemetry, and image pushing is coherent with the command’s functionality.common/docker/client_container.go (1)
18-18: Good refactoring to use forge.NewTeaProgram.This change improves terminal handling by using the custom wrapper function that properly handles non-terminal environments. This ensures better compatibility across different runtime environments.
Also applies to: 41-41
common/docker/client_image.go (1)
25-25: Good refactoring to use forge.NewTeaProgram.Consistent with changes in other files, this refactoring improves terminal handling by using the custom wrapper that handles non-terminal environments properly.
Also applies to: 53-53
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/world/forge/forge_test.go (1)
1590-1590: Consider refactoring this complex test function.The
//nolint:gocognitcomment is suppressing cognitive complexity warnings, which indicates the function might be too complex. While test functions often have higher complexity due to test case setup and assertions, consider breaking this test down into smaller, more focused tests for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
cmd/world/forge/forge_test.go(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
cmd/world/forge/forge_test.go (1)
cmd/world/forge/project.go (3) (3)
regionSelector(21-21)err(549-549)createProject(216-296)
🔇 Additional comments (3)
cmd/world/forge/forge_test.go (3)
1748-1750: Good defensive coding with nil check.Adding a nil check for
regionSelectorbefore operating on it is a great improvement that prevents potential nil pointer dereferences and provides informative feedback when initialization fails.
1759-1766: Improved error handling for region selector.This conditional block adds a proper nil check before sending actions to the region selector, making the test more robust against failures. The additional debug message helps identify when the region selector is not properly initialized.
1773-1775: Enhanced test reliability with require assertions.Replacing
s.NoError(err)and explicit nil checks withs.Require().NoError(err)ands.Require().NotNil(prj)is an improvement that ensures the test fails immediately if these critical checks don't pass, preventing subsequent code from running with invalid values.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
common/docker/client_image.go (2)
499-507: Consider tagging images with the destination registry before pushing.The current implementation checks if images exist locally but doesn't tag them with the destination registry. This could cause issues if
pushTois different from the original image name.Consider adding image tagging before pushing:
// check if the image exists _, _, err := c.client.ImageInspectWithRaw(ctx, imageName) if err != nil { return eris.New(fmt.Sprintf("Error inspecting image %s for service %s: %v\n", imageName, service.Name, err)) } + + // Tag the image with the destination registry if needed + destinationImage := fmt.Sprintf("%s/%s", pushTo, strings.Split(imageName, "/")[len(strings.Split(imageName, "/"))-1]) + if err := c.client.ImageTag(ctx, imageName, destinationImage); err != nil { + return eris.Wrap(err, fmt.Sprintf("Failed to tag image %s as %s", imageName, destinationImage)) + }
536-576: Consider extracting common Docker event handling logic.There's significant code duplication between this event handling logic and the one in
pullImages. Consider refactoring to extract the common pattern into a utility function.Create a shared function to handle Docker operation progress:
+// processDockerStreamEvents handles events from Docker stream operations like pull, push, etc. +// It updates the progress bar based on the events and collects errors. +func processDockerStreamEvents( + ctx context.Context, + decoder *json.Decoder, + bar *mpb.Bar, + imageName string, + operation string, + errChan chan<- error) { + var current int + var event map[string]interface{} + for decoder.More() { + select { + case <-ctx.Done(): + fmt.Printf("%s of image %s was canceled\n", operation, imageName) + bar.Abort(false) + return + default: + if err := decoder.Decode(&event); err != nil { + errChan <- eris.New(fmt.Sprintf("Error decoding event for %s: %v\n", imageName, err)) + continue + } + + // Check for errorDetail and error fields + if errorDetail, ok := event["errorDetail"]; ok { + if errorMessage, ok := errorDetail.(map[string]interface{})["message"]; ok { + errChan <- eris.New(errorMessage.(string)) + continue + } + } else if errorMsg, ok := event["error"]; ok { + errChan <- eris.New(errorMsg.(string)) + continue + } + + // Handle progress updates + if progressDetail, ok := event["progressDetail"].(map[string]interface{}); ok { + if total, ok := progressDetail["total"].(float64); ok && total > 0 { + calculatedCurrent := int(progressDetail["current"].(float64) * 100 / total) + if calculatedCurrent > current { + bar.SetCurrent(int64(calculatedCurrent)) + current = calculatedCurrent + } + } + } + } + } + bar.SetCurrent(100) +}Then use this function in both
pullImagesandpushImages:- decoder := json.NewDecoder(responseBody) - var current int - var event map[string]interface{} - for decoder.More() { //nolint:dupl // different commands - // ... existing event handling logic - } - - // Finish the progress bar - bar.SetCurrent(100) //nolint:gomnd + processDockerStreamEvents(ctx, json.NewDecoder(responseBody), bar, imageName, "Pushing", errChan)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
common/docker/client_image.go(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
common/docker/client_image.go (2)
common/docker/client.go (8) (8)
c(68-70)c(72-109)c(111-166)c(168-183)c(185-205)c(207-216)c(218-268)Client(48-51)common/docker/service/service.go (1) (1)
Service(25-38)
🔇 Additional comments (2)
common/docker/client_image.go (2)
423-423: Appropriate linting suppression.The linting suppression for the duplicate code is well-documented, explaining that although similar patterns exist, they serve different commands.
486-601: Well-structured implementation of thepushImagesmethod.The implementation properly handles concurrent image pushing with progress tracking, error collection, and provides a clean interface that integrates well with the existing
Buildmethod.
Closes: PLAT-202
Summary by CodeRabbit