Support --profile flag for custom commands with autocomplete#2190
Open
Support --profile flag for custom commands with autocomplete#2190
Conversation
…complete ## What Changed - Added early parsing of --profile flag (before Cobra parses) to ensure profiles are applied during initial config load, so custom commands get a properly profiled atmosConfig - Exported ParseProfilesFromOsArgs() and ParseProfilesFromEnvString() functions for use during early boot - Added profile autocomplete by implementing profileFlagCompletion() that uses the profile manager to list available profiles - Extended StringSliceFlag to support custom completion functions (via CompletionFunc field) - Updated FlagRegistry.SetCompletionFunc() to support both StringFlag and StringSliceFlag types ## Why Previously, custom commands could accept --profile but the flag was never applied because: 1. atmosConfig was loaded at boot time (before Cobra parses flags) 2. Custom commands captured this pre-profile config in their closure 3. Built-in commands work because they call GetConfigAndStacksInfo() which re-extracts --profile during command execution This fix follows the same pattern as --chdir: parse the flag early from os.Args and pass it to InitCliConfig, ensuring all commands (built-in and custom) get a properly profiled configuration. The autocomplete feature provides users with shell suggestions of available profiles when typing atmos commands. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Contributor
📝 WalkthroughWalkthroughIntegrates Atmos profile handling into CLI startup: profiles parsed from args/env before config load, passed into InitCliConfig; adds exported profile parsing helpers; extends flag registry and StringSliceFlag to support shell completion for the global Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Runtime
participant Root as cmd/root.go
participant Parser as pkg/config
participant Registry as pkg/flags
participant ProfileMgr as ProfileManager
CLI->>Root: start (args, env)
Root->>Parser: ParseProfilesFromOsArgs(os.Args) / ParseProfilesFromEnvString(env)
Parser-->>Root: profiles[]
Root->>Parser: InitCliConfig(with ProfilesFromArg, AtmosBasePath, config selections)
Parser-->>Root: config initialized
CLI->>Registry: request shell completion for --profile
Registry->>ProfileMgr: list profiles (using initialized config)
ProfileMgr-->>Registry: profile names
Registry-->>CLI: completion suggestions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/flags/registry_test.go (1)
148-153: Prefer asserting through theFlagAPI, not concrete struct fields.These assertions currently couple to implementation details (
flag.CompletionFunc). Usingregistry.Get(...).GetCompletionFunc()would keep the test behavior-focused and more refactor-safe.As per coding guidelines "Test Quality (MANDATORY): Test behavior, not implementation."
Also applies to: 165-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/flags/registry_test.go` around lines 148 - 153, The test is asserting on the concrete StringFlag.CompletionFunc field; instead use the Flag API accessor to avoid coupling to implementation details—replace uses of registry.Get("stack").(*StringFlag).CompletionFunc with registry.Get("stack").GetCompletionFunc() (or the equivalent interface method) and call that to obtain (values, directive) for assertions, applying the same change for the other occurrence around the second block (currently lines 165-170).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/root.go`:
- Around line 1866-1867: The completion config is being initialized with an
empty schema.ConfigAndStacksInfo which ignores parsed global flags; before
calling cfg.InitCliConfig(schema.ConfigAndStacksInfo{}, false) populate a
ConfigAndStacksInfo instance by calling flags.ParseGlobalFlags(cmd, v) (or
equivalent) so the global flag values are applied, then pass that populated
struct to cfg.InitCliConfig; update the call around atmosCfg, err :=
cfg.InitCliConfig(...) to use the parsed/filled ConfigAndStacksInfo instead of
the empty literal.
---
Nitpick comments:
In `@pkg/flags/registry_test.go`:
- Around line 148-153: The test is asserting on the concrete
StringFlag.CompletionFunc field; instead use the Flag API accessor to avoid
coupling to implementation details—replace uses of
registry.Get("stack").(*StringFlag).CompletionFunc with
registry.Get("stack").GetCompletionFunc() (or the equivalent interface method)
and call that to obtain (values, directive) for assertions, applying the same
change for the other occurrence around the second block (currently lines
165-170).
🪄 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
Run ID: 9dfd34d3-4eeb-4824-98a2-2b5cbc6335d8
📒 Files selected for processing (8)
cmd/root.gopkg/config/load.gopkg/config/load_flags_test.gopkg/config/load_profile_test.gopkg/config/load_test.gopkg/flags/registry.gopkg/flags/registry_test.gopkg/flags/types.go
The pflag.FlagSet created in ParseProfilesFromOsArgs() defaulted to writing usage output to os.Stderr, causing "Usage of profile-parser" to appear on every CLI invocation and breaking all snapshot tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Parse global flags (--base-path, --config, --config-path, --profile) before initializing config in profileFlagCompletion, matching the pattern used by other completion functions (e.g., ansible). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/root.go`:
- Around line 1403-1421: The InitCliConfig call is being passed only
ProfilesFromArg, so early config-selection flags (--base-path, --config,
--config-path) are ignored when building custom commands; update the pre-Cobra
parsing to extract those flags (using the same early-parsing pattern as
ParseProfilesFromOsArgs/ParseProfilesFromEnvString) and populate the
corresponding fields on the schema.ConfigAndStacksInfo passed into
cfg.InitCliConfig (e.g., set BasePath, Config, ConfigPath or whatever selection
fields exist on schema.ConfigAndStacksInfo) so InitCliConfig/LoadConfig sees the
intended selection before Cobra loads commands.
🪄 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
Run ID: 59e8f018-ac3e-476f-b5c3-f7b39be3f7e2
📒 Files selected for processing (2)
cmd/root.gopkg/config/load.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/config/load.go
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (71.79%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2190 +/- ##
==========================================
+ Coverage 77.24% 77.29% +0.05%
==========================================
Files 959 960 +1
Lines 90782 91158 +376
==========================================
+ Hits 70124 70465 +341
- Misses 16574 16609 +35
Partials 4084 4084
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ig call The early InitCliConfig call only seeded ProfilesFromArg, so config-selection flags were ignored when building custom commands and aliases. Now all three config-selection flags are parsed early from os.Args (with env var fallbacks) and passed into InitCliConfig, ensuring `atmos --config ... --profile ... <cmd>` resolves the correct configuration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/root.go`:
- Around line 1415-1417: The current logic replaces configSel with
cfg.ConfigSelectionFromEnv() only when all selectors are empty; instead fetch
envSel := cfg.ConfigSelectionFromEnv() and for each field (configSel.BasePath,
configSel.Config, configSel.ConfigPath, etc.) if that field is empty/zero then
assign the corresponding value from envSel, so environment variables are applied
per-field (flags keep precedence). Use the existing ConfigSelectionFromEnv() and
update only missing fields on configSel to preserve flags > env > file >
defaults ordering.
In `@pkg/config/load_config_selection_test.go`:
- Around line 169-177: The subtests in load_config_selection_test.go can inherit
host environment variables causing flaky failures; before applying each test
case's tt.envVars in the t.Run block, explicitly clear ATMOS_BASE_PATH,
ATMOS_CONFIG, and ATMOS_CONFIG_PATH (use t.Setenv(key, "") or equivalent) so
ConfigSelectionFromEnv() reads only the intended values for each subtest; update
the loop around ConfigSelectionFromEnv() / assert.Equal checks to first reset
those three env vars, then apply tt.envVars, then call ConfigSelectionFromEnv().
🪄 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
Run ID: ec0926a0-5c5c-4d5a-ab32-17e1b60ac73d
📒 Files selected for processing (3)
cmd/root.gopkg/config/load.gopkg/config/load_config_selection_test.go
Comment on lines
+1415
to
+1417
| if configSel.BasePath == "" && len(configSel.Config) == 0 && len(configSel.ConfigPath) == 0 { | ||
| configSel = cfg.ConfigSelectionFromEnv() | ||
| } |
Contributor
There was a problem hiding this comment.
Apply env fallback per field, not only when all selectors are empty.
At Line 1415, env fallback is skipped if any selector flag is present. Example: --base-path plus ATMOS_CONFIG_PATH ignores ATMOS_CONFIG_PATH, which breaks expected precedence behavior.
Proposed patch
configSel := cfg.ParseConfigSelectionFromOsArgs(os.Args[1:])
- if configSel.BasePath == "" && len(configSel.Config) == 0 && len(configSel.ConfigPath) == 0 {
- configSel = cfg.ConfigSelectionFromEnv()
- }
+ envSel := cfg.ConfigSelectionFromEnv()
+ if configSel.BasePath == "" {
+ configSel.BasePath = envSel.BasePath
+ }
+ if len(configSel.Config) == 0 {
+ configSel.Config = envSel.Config
+ }
+ if len(configSel.ConfigPath) == 0 {
+ configSel.ConfigPath = envSel.ConfigPath
+ }As per coding guidelines: "Support configuration via files, environment variables, and flags following the precedence order: flags > environment variables > config file > defaults".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/root.go` around lines 1415 - 1417, The current logic replaces configSel
with cfg.ConfigSelectionFromEnv() only when all selectors are empty; instead
fetch envSel := cfg.ConfigSelectionFromEnv() and for each field
(configSel.BasePath, configSel.Config, configSel.ConfigPath, etc.) if that field
is empty/zero then assign the corresponding value from envSel, so environment
variables are applied per-field (flags keep precedence). Use the existing
ConfigSelectionFromEnv() and update only missing fields on configSel to preserve
flags > env > file > defaults ordering.
Comment on lines
+169
to
+177
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| for k, v := range tt.envVars { | ||
| t.Setenv(k, v) | ||
| } | ||
| sel := ConfigSelectionFromEnv() | ||
| assert.Equal(t, tt.basePath, sel.BasePath, "BasePath") | ||
| assert.Equal(t, tt.config, sel.Config, "Config") | ||
| assert.Equal(t, tt.configPath, sel.ConfigPath, "ConfigPath") |
Contributor
There was a problem hiding this comment.
Clear target env vars per subtest to avoid ambient-env flakes.
At Line 170, cases like “no env vars” can inherit host env values. Reset ATMOS_BASE_PATH, ATMOS_CONFIG, and ATMOS_CONFIG_PATH to empty at subtest start.
Proposed patch
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
+ t.Setenv("ATMOS_BASE_PATH", "")
+ t.Setenv("ATMOS_CONFIG", "")
+ t.Setenv("ATMOS_CONFIG_PATH", "")
+
for k, v := range tt.envVars {
t.Setenv(k, v)
}
sel := ConfigSelectionFromEnv()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| for k, v := range tt.envVars { | |
| t.Setenv(k, v) | |
| } | |
| sel := ConfigSelectionFromEnv() | |
| assert.Equal(t, tt.basePath, sel.BasePath, "BasePath") | |
| assert.Equal(t, tt.config, sel.Config, "Config") | |
| assert.Equal(t, tt.configPath, sel.ConfigPath, "ConfigPath") | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| t.Setenv("ATMOS_BASE_PATH", "") | |
| t.Setenv("ATMOS_CONFIG", "") | |
| t.Setenv("ATMOS_CONFIG_PATH", "") | |
| for k, v := range tt.envVars { | |
| t.Setenv(k, v) | |
| } | |
| sel := ConfigSelectionFromEnv() | |
| assert.Equal(t, tt.basePath, sel.BasePath, "BasePath") | |
| assert.Equal(t, tt.config, sel.Config, "Config") | |
| assert.Equal(t, tt.configPath, sel.ConfigPath, "ConfigPath") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/config/load_config_selection_test.go` around lines 169 - 177, The
subtests in load_config_selection_test.go can inherit host environment variables
causing flaky failures; before applying each test case's tt.envVars in the t.Run
block, explicitly clear ATMOS_BASE_PATH, ATMOS_CONFIG, and ATMOS_CONFIG_PATH
(use t.Setenv(key, "") or equivalent) so ConfigSelectionFromEnv() reads only the
intended values for each subtest; update the loop around
ConfigSelectionFromEnv() / assert.Equal checks to first reset those three env
vars, then apply tt.envVars, then call ConfigSelectionFromEnv().
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
1 participant
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.
what
--profileflag (before Cobra parses) to ensure profiles are applied during initial config load, so custom commands receive a properly profiledatmosConfigStringSliceFlagto support custom completion functionsFlagRegistry.SetCompletionFunc()to support bothStringFlagandStringSliceFlagtypeswhy
Previously, custom commands could accept
--profilebut the flag was never applied becauseatmosConfigwas loaded at boot time (before Cobra parsed flags). Custom commands captured this pre-profile config in their closure. Built-in commands worked because they calledGetConfigAndStacksInfo()which re-extracted--profileduring command execution.This fix follows the same pattern as
--chdir: parse the flag early fromos.Argsand pass it toInitCliConfig, ensuring all commands (built-in and custom) get properly profiled configuration.The autocomplete feature provides users with shell suggestions of available profiles when typing atmos commands.
references
Resolves the gap where custom commands couldn't use the
--profileflag effectively.Summary by CodeRabbit
New Features
Tests