env vars resolution summary #1728
Conversation
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as CLI
participant Commands as commands.go
participant Loader as Loader
participant Provider as Provider
participant Project as Compose Project
participant ComposePkg as compose.PrintConfigResolutionSummary
CLI->>Commands: run `config resolve`
Commands->>Loader: load environment & loader
Loader-->>Commands: loader instance
Commands->>Provider: create provider with loader
Provider-->>Commands: provider ready
Commands->>Provider: LoadProject(ctx)
Provider-->>Project: project data
Commands->>Provider: ListConfigNames(ctx)
Provider-->>Commands: []configNames
Commands->>ComposePkg: PrintConfigResolutionSummary(project, configNames)
ComposePkg->>Project: inspect services & envs
ComposePkg->>ComposePkg: determineConfigSource(...) (masking/interpolation)
ComposePkg-->>Commands: printed summary / result
Commands-->>CLI: exit/status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (1)
src/pkg/cli/compose/validation.go (1)
475-478: Consider the side effect of printing during validation.The call to
PrintConfigResolutionSummaryintroduces output during validation, which means any code path callingValidateProjectConfigwill now print this summary. This may be intentional for the newconfig resolvecommand, but could result in unexpected output in other contexts.Consider whether:
- This is the intended behavior for all validation paths, or
- The summary should only print for the explicit
config resolvecommandIf the latter, you might want to return the summary data from validation and let callers decide when to print it.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/cmd/cli/command/commands.go(2 hunks)src/pkg/cli/compose/configResolution.go(1 hunks)src/pkg/cli/compose/validation.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/pkg/cli/compose/validation.go (1)
src/pkg/cli/compose/configResolution.go (1)
PrintConfigResolutionSummary(68-99)
src/cmd/cli/command/commands.go (2)
src/protos/io/defang/v1/fabric.pb.go (3)
ListConfigsRequest(2734-2739)ListConfigsRequest(2752-2752)ListConfigsRequest(2767-2769)src/pkg/cli/compose/configResolution.go (1)
PrintConfigResolutionSummary(68-99)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
src/cmd/cli/command/commands.go (2)
301-302: LGTM! Command registration is correct.The new
config resolvesubcommand is properly registered with the parentconfigCmd, following the established pattern for other config subcommands.
975-1001: API usage verified. Theprovider.ListConfigmethod correctly accepts*defangv1.ListConfigsRequestand returns*defangv1.Secretswith aNamesfield, as used on line 999.src/pkg/cli/compose/validation.go (2)
438-450: LGTM! Clean extraction of interpolation variables.The function correctly extracts variable names from the interpolation regex matches. The implementation is reusable and follows good practices.
461-462: LGTM! Proper use of the new helper function.The call to
DetectInterpolationVariablesis safe (after the nil check on line 457) and improves code maintainability by eliminating duplicate logic.src/pkg/cli/compose/configResolution.go (5)
9-14: LGTM! Well-structured output type.The
configOutputstruct is properly defined with appropriate JSON tags andomitemptyfor optional fields.
16-37: LGTM! Proper enum implementation with Stringer interface.The
Sourceenum and itsString()method are well-implemented, following Go best practices. The fallback toSourceUnknownensures safety for unexpected values.
41-64: LGTM! Sound logic for determining config source.The function correctly identifies the source of environment variables and applies masking for sensitive defang config values. The handling of nil values and interpolation references is appropriate.
66-66: LGTM! Appropriate masking constant.The masked value provides a clear indication that sensitive information is hidden.
68-99: Verify the Project type definition is not a concern.The
Projecttype is properly defined in an external compose package (likelygithub.com/compose-spec/compose-go/typesor equivalent from Docker CLI) and includes theServicesfield used by this function. No additional verification or changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pkg/cli/composeUp.go (1)
73-78: Consider adding context separation for better UX.The config resolution summary is correctly printed after successful validation. However, consider adding a blank line before the summary for better visual separation in the terminal output, especially if other messages are printed before it.
🔎 Optional: Add visual separation
} // Print config resolution summary + term.Println() // Add blank line for visual separation err = compose.PrintConfigResolutionSummary(*project, listConfigNames) if err != nil { return nil, project, err }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/cmd/cli/command/commands.go(3 hunks)src/pkg/cli/compose/validation.go(2 hunks)src/pkg/cli/compose/validation_test.go(5 hunks)src/pkg/cli/composeUp.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pkg/cli/compose/validation.go
🧰 Additional context used
🧬 Code graph analysis (3)
src/cmd/cli/command/commands.go (3)
src/pkg/cli/compose/validation.go (1)
ValidateProjectConfig(452-486)src/protos/io/defang/v1/fabric.pb.go (3)
ListConfigsRequest(2734-2739)ListConfigsRequest(2752-2752)ListConfigsRequest(2767-2769)src/pkg/cli/compose/configResolution.go (1)
PrintConfigResolutionSummary(68-99)
src/pkg/cli/composeUp.go (2)
src/pkg/cli/compose/validation.go (1)
ValidateProjectConfig(452-486)src/pkg/cli/compose/configResolution.go (1)
PrintConfigResolutionSummary(68-99)
src/pkg/cli/compose/validation_test.go (1)
src/pkg/cli/compose/validation.go (1)
ValidateProjectConfig(452-486)
🔇 Additional comments (6)
src/pkg/cli/compose/validation_test.go (2)
59-67: LGTM! Refactoring aligns with updated validation signature.The change from lazy function-based config name provision to eager fetching is correct. Error handling is appropriate, and the refactored code properly passes the config names slice to
ValidateProjectConfig.
102-102: LGTM! Test cases properly updated for new signature.All test invocations of
ValidateProjectConfigcorrectly pass explicit[]stringslices, matching the updated function signature. The test scenarios (empty configs, missing configs, valid configs) remain logically consistent.Also applies to: 117-117, 140-140, 152-152
src/cmd/cli/command/commands.go (3)
301-302: LGTM! Command properly wired into CLI structure.The
configResolveCmdis correctly added as a subcommand toconfigCmd.
973-999: LGTM! New command implementation follows established patterns.The
config resolvecommand is well-structured and follows the same pattern as other commands in this file. It properly:
- Requires authentication
- Loads the project and provider
- Fetches config names from the provider
- Prints the resolution summary
The error handling is appropriate at each step.
747-747: LGTM! Updated to match new validation signature.The call to
ValidateProjectConfigis correctly updated to pass an empty slice[]string{}, which makes sense in this context—the function is collecting unset environment variables by intentionally validating against no configs.src/pkg/cli/composeUp.go (1)
64-71: LGTM! Proper refactoring to eager config name fetching.The change correctly:
- Eagerly fetches config names before validation
- Handles errors immediately with early return
- Passes the concrete slice to
ValidateProjectConfig- Wraps validation errors in
ComposeErrorfor proper error classification
Description
This pull request introduces a new command for resolving and displaying the final configuration of environment variables in a Compose project or from
defang config set.Enhanced the
ComposeUpworkflow to print the config resolution summary after validation, providing users with immediate feedback on how environment variables are resolved.Example:
TODO: write unit test
Linked Issues
#1481
Checklist
Summary by CodeRabbit
New Features
config resolveCLI subcommand (alias:final) to show a final configuration summary table with where environment variables come from and their resolved values (masked for sensitive configs).Bug Fixes / Improvements
✏️ Tip: You can customize this high-level summary in your review settings.