-
-
Notifications
You must be signed in to change notification settings - Fork 112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve atmos list stacks #979
base: main
Are you sure you want to change the base?
Changes from all commits
8311350
faeec51
80d540c
baa42e5
060d49b
c29f1e8
d53379f
9e79f60
6bbe5b7
1611285
8690e35
d7718bc
6c33077
1a871fa
7ad8f6e
856c0eb
d66c1c7
6b257de
c976cf3
c8dd900
14e477f
98f5dfe
da45ed2
aab2f73
6ead9fb
90345ad
e348298
c0d58e0
7d23cf2
7468739
e69f7af
de25bdb
4fcbaca
72fa8a0
e63a8d0
d25dd4b
07d2a69
4ed7c5f
61e6383
049a2ed
d6c7f91
7429e5c
3167c4f
8e3dd3b
b669f45
bac6f41
0a0ad12
53bd07c
21a2de6
22154ad
35710f0
a2fce86
8ee545c
0de25ea
f1b682b
43fcbaa
42a0938
132d9a0
7bee512
80c577b
7b403ba
1eeb293
7934498
53a10b7
108ad37
d6e3919
21c21f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -394,4 +394,3 @@ version: | |
enabled: true | ||
timeout: 1000 # ms | ||
frequency: 1h | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -704,12 +704,36 @@ | |
u.PrintInvalidUsageErrorAndExit(errors.New(details), suggestion) | ||
} | ||
|
||
// filterStackNames filters out file paths and returns valid stack names | ||
Check failure on line 707 in cmd/cmd_utils.go
|
||
func filterStackNames(stacksMap map[string]any, basePath string) []string { | ||
stackNames := make([]string, 0) | ||
for stackName := range stacksMap { | ||
// Skip if it's a file path (starts with base path) | ||
if basePath != "" && strings.HasPrefix(stackName, basePath+"/") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this is not right either. Stacks can exist anywhere, what we include and exclude is entirely config driven. We are not consulting that configuration, and deriving an implementation that doesn’t correspond to the documentation. We should have a centralized function for this and it relates to stacks not general utils, IMO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so I think on this case would be best having that function inside stacks pkg and then reuse FindAllStackConfigsInPaths from config to find the paths There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aknysh please let me know your opinion of whats the best approach to refactor this, I see that we also have getConfigAndStacksInfo in utils |
||
continue | ||
} | ||
stackNames = append(stackNames, stackName) | ||
} | ||
return stackNames | ||
} | ||
|
||
func stackFlagCompletion(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { | ||
output, err := listStacks(cmd) | ||
configAndStacksInfo := schema.ConfigAndStacksInfo{} | ||
atmosConfig, err := cfg.InitCliConfig(configAndStacksInfo, true) | ||
if err != nil { | ||
return nil, cobra.ShellCompDirectiveNoFileComp | ||
} | ||
|
||
// Get all stacks | ||
stacksMap, err := e.ExecuteDescribeStacks(atmosConfig, "", nil, nil, nil, false, true, true, false, nil) | ||
if err != nil { | ||
return nil, cobra.ShellCompDirectiveNoFileComp | ||
} | ||
return output, cobra.ShellCompDirectiveNoFileComp | ||
|
||
// Filter stack names using configured base path | ||
stackNames := filterStackNames(stacksMap, atmosConfig.Stacks.BasePath) | ||
|
||
return stackNames, cobra.ShellCompDirectiveNoFileComp | ||
} | ||
|
||
func AddStackCompletion(cmd *cobra.Command) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
package cmd | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestFilterStackNames(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
stacksBasePath string | ||
stacksMap map[string]any | ||
expectedStacks []string | ||
}{ | ||
{ | ||
name: "filters out paths with configured base path", | ||
stacksBasePath: "stacks", | ||
stacksMap: map[string]any{ | ||
"stacks/test1.yaml": nil, | ||
"test1": nil, | ||
"test2": nil, | ||
"stacks/test2": nil, | ||
}, | ||
expectedStacks: []string{"test1", "test2"}, | ||
}, | ||
{ | ||
name: "handles custom base path", | ||
stacksBasePath: "custom/path", | ||
stacksMap: map[string]any{ | ||
"custom/path/test1.yaml": nil, | ||
"test1": nil, | ||
"test2": nil, | ||
"custom/path/test2": nil, | ||
}, | ||
expectedStacks: []string{"test1", "test2"}, | ||
}, | ||
{ | ||
name: "handles empty base path", | ||
stacksBasePath: "", | ||
stacksMap: map[string]any{ | ||
"test1.yaml": nil, | ||
"test1": nil, | ||
"test2": nil, | ||
}, | ||
expectedStacks: []string{"test1", "test2", "test1.yaml"}, | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
// Run the filter function | ||
stacks := filterStackNames(tt.stacksMap, tt.stacksBasePath) | ||
|
||
// Verify the results | ||
assert.ElementsMatch(t, tt.expectedStacks, stacks) | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -214,6 +214,7 @@ | |
if !u.MapKeyExists(finalStacksMap, stackName) { | ||
finalStacksMap[stackName] = make(map[string]any) | ||
finalStacksMap[stackName].(map[string]any)["components"] = make(map[string]any) | ||
finalStacksMap[stackName].(map[string]any)["atmos_stack_file"] = stackFileName | ||
} | ||
|
||
if componentsSection, ok := stackSection.(map[string]any)["components"].(map[string]any); ok { | ||
|
@@ -307,15 +308,22 @@ | |
if err != nil { | ||
return nil, err | ||
} | ||
} else { | ||
} else if atmosConfig.Stacks.NamePattern != "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aknysh please check this part. We need to ensure that both name pattern and name templates work. |
||
context = cfg.GetContextFromVars(varsSection) | ||
configAndStacksInfo.Context = context | ||
stackName, err = cfg.GetContextPrefix(stackFileName, context, GetStackNamePattern(atmosConfig), stackFileName) | ||
if err != nil { | ||
return nil, err | ||
} | ||
} else { | ||
// If no name pattern or template is configured, use the stack file name | ||
stackName = stackFileName | ||
} | ||
|
||
// Update the component section with the final stack name | ||
configAndStacksInfo.ComponentSection["atmos_stack"] = stackName | ||
configAndStacksInfo.ComponentSection["stack"] = stackName | ||
|
||
if filterByStack != "" && filterByStack != stackFileName && filterByStack != stackName { | ||
continue | ||
} | ||
|
@@ -327,18 +335,15 @@ | |
// Only create the stack entry if it doesn't exist | ||
if !u.MapKeyExists(finalStacksMap, stackName) { | ||
finalStacksMap[stackName] = make(map[string]any) | ||
finalStacksMap[stackName].(map[string]any)["components"] = make(map[string]any) | ||
finalStacksMap[stackName].(map[string]any)["atmos_stack_file"] = stackFileName | ||
} | ||
|
||
configAndStacksInfo.ComponentSection["atmos_component"] = componentName | ||
configAndStacksInfo.ComponentSection["atmos_stack"] = stackName | ||
configAndStacksInfo.ComponentSection["stack"] = stackName | ||
configAndStacksInfo.ComponentSection["atmos_stack_file"] = stackFileName | ||
configAndStacksInfo.ComponentSection["atmos_manifest"] = stackFileName | ||
|
||
if len(components) == 0 || u.SliceContainsString(components, componentName) || u.SliceContainsString(derivedComponents, componentName) { | ||
if !u.MapKeyExists(finalStacksMap[stackName].(map[string]any), "components") { | ||
finalStacksMap[stackName].(map[string]any)["components"] = make(map[string]any) | ||
} | ||
if !u.MapKeyExists(finalStacksMap[stackName].(map[string]any)["components"].(map[string]any), "terraform") { | ||
finalStacksMap[stackName].(map[string]any)["components"].(map[string]any)["terraform"] = make(map[string]any) | ||
} | ||
|
@@ -516,15 +521,22 @@ | |
if err != nil { | ||
return nil, err | ||
} | ||
} else { | ||
} else if atmosConfig.Stacks.NamePattern != "" { | ||
context = cfg.GetContextFromVars(varsSection) | ||
configAndStacksInfo.Context = context | ||
stackName, err = cfg.GetContextPrefix(stackFileName, context, GetStackNamePattern(atmosConfig), stackFileName) | ||
if err != nil { | ||
return nil, err | ||
} | ||
} else { | ||
// If no name pattern or template is configured, use the stack file name | ||
stackName = stackFileName | ||
} | ||
|
||
// Update the component section with the final stack name | ||
configAndStacksInfo.ComponentSection["atmos_stack"] = stackName | ||
configAndStacksInfo.ComponentSection["stack"] = stackName | ||
Check failure on line 538 in internal/exec/describe_stacks.go
|
||
|
||
if filterByStack != "" && filterByStack != stackFileName && filterByStack != stackName { | ||
continue | ||
} | ||
|
@@ -536,18 +548,15 @@ | |
// Only create the stack entry if it doesn't exist | ||
if !u.MapKeyExists(finalStacksMap, stackName) { | ||
finalStacksMap[stackName] = make(map[string]any) | ||
finalStacksMap[stackName].(map[string]any)["components"] = make(map[string]any) | ||
finalStacksMap[stackName].(map[string]any)["atmos_stack_file"] = stackFileName | ||
} | ||
|
||
configAndStacksInfo.ComponentSection["atmos_component"] = componentName | ||
configAndStacksInfo.ComponentSection["atmos_stack"] = stackName | ||
configAndStacksInfo.ComponentSection["stack"] = stackName | ||
configAndStacksInfo.ComponentSection["atmos_stack_file"] = stackFileName | ||
configAndStacksInfo.ComponentSection["atmos_manifest"] = stackFileName | ||
|
||
if len(components) == 0 || u.SliceContainsString(components, componentName) || u.SliceContainsString(derivedComponents, componentName) { | ||
if !u.MapKeyExists(finalStacksMap[stackName].(map[string]any), "components") { | ||
finalStacksMap[stackName].(map[string]any)["components"] = make(map[string]any) | ||
} | ||
if !u.MapKeyExists(finalStacksMap[stackName].(map[string]any)["components"].(map[string]any), "helmfile") { | ||
finalStacksMap[stackName].(map[string]any)["components"].(map[string]any)["helmfile"] = make(map[string]any) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s revise this function name. We want to add proper filtering later (not this PR), but this is filtering on paths. Also, it’s not clear to me why we need this. Can you add a comment explaining why we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this wold get replaced once we refactor the full stacks path on the discussion bellow