added stage field in spec and solved issue of checking duplicates of multip… #1875
Conversation
…le stages solves issue: diggerhq#1795
WalkthroughThe changes modify the variable specification handling in the backend services. A new Changes
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![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 (1)
backend/services/spec.go (1)
118-120: Consider using constants for stage names.The stage names are currently hardcoded strings. To prevent inconsistencies and improve maintainability, consider defining these as constants.
+const ( + StageState = "state" + StageCommands = "commands" + StageRun = "run" +) - stateVariables := getVariablesSpecFromEnvMap(jobSpec.StateEnvVars, "state") - commandVariables := getVariablesSpecFromEnvMap(jobSpec.CommandEnvVars, "commands") - runVariables := getVariablesSpecFromEnvMap(jobSpec.RunEnvVars, "run") + stateVariables := getVariablesSpecFromEnvMap(jobSpec.StateEnvVars, StageState) + commandVariables := getVariablesSpecFromEnvMap(jobSpec.CommandEnvVars, StageCommands) + runVariables := getVariablesSpecFromEnvMap(jobSpec.RunEnvVars, StageRun)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/services/spec.go(3 hunks)libs/spec/models.go(1 hunks)
🔇 Additional comments (3)
libs/spec/models.go (1)
48-48: LGTM! Stage field addition looks good.The new
Stagefield is properly added to theVariableSpecstruct with appropriate JSON tagging, aligning with the PR's objective to support stage-specific variable handling.backend/services/spec.go (2)
Line range hint
63-85: LGTM! Stage parameter properly integrated.The function now correctly sets the stage field for both interpolated and non-interpolated variables.
122-130: LGTM! Stage-specific duplicate checking is well implemented.The duplicate checking is properly implemented for each stage before combining the variables, which aligns with the PR's objective of solving the issue of checking duplicates across multiple stages.
backend/services/spec.go
Outdated
| func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string) (error) { | ||
| // Extract the names from VariableSpec | ||
| justNames := lo.Map(variablesSpec, func(item VariableSpec, i int) string { | ||
| return item.Name | ||
| }) | ||
|
|
||
| // Group names by their occurrence | ||
| nameCounts := lo.CountValues(justNames) | ||
|
|
||
| // Filter names that occur more than once | ||
| duplicates := lo.Keys(lo.Filter(nameCounts, func(count int, name string) bool { | ||
| return count > 1 | ||
| })) | ||
|
|
||
| if len(duplicates) > 0 { | ||
| return fmt.Errorf("In %v stage, found duplicate variables: %v", stage, duplicates) | ||
| } | ||
|
|
||
| return nil // No duplicates found | ||
| } |
There was a problem hiding this comment.
Fix compilation issues and improve error handling in duplicate check function.
There are several issues in this function:
- The function is defined but never used (GetSpecFromJob calls
checkDuplicatesInStageinstead) - The Map function uses undefined type
VariableSpecinstead ofspec.VariableSpec - The error message could be more specific
Apply these fixes:
-func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string) (error) {
+func checkDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string) error {
// Extract the names from VariableSpec
- justNames := lo.Map(variablesSpec, func(item VariableSpec, i int) string {
+ justNames := lo.Map(variablesSpec, func(item spec.VariableSpec, i int) string {
return item.Name
})
// Group names by their occurrence
nameCounts := lo.CountValues(justNames)
// Filter names that occur more than once
duplicates := lo.Keys(lo.Filter(nameCounts, func(count int, name string) bool {
return count > 1
}))
if len(duplicates) > 0 {
- return fmt.Errorf("In %v stage, found duplicate variables: %v", stage, duplicates)
+ return fmt.Errorf("duplicate variable names found in '%s' stage: %v", stage, strings.Join(duplicates, ", "))
}
return nil // No duplicates found
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/services/spec.go (2)
89-108: Remove unnecessary parentheses in function signature.The function implementation looks good, but there's a minor style improvement needed.
-func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string) (error) { +func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string) error {
118-130: Consider defining stage names as constants.The stage names ("state", "commands", "run") are used multiple times. To prevent typos and ensure consistency, consider defining them as package-level constants.
+const ( + StageState = "state" + StageCommands = "commands" + StageRun = "run" +) - stateVariables := getVariablesSpecFromEnvMap(jobSpec.StateEnvVars, "state") - commandVariables := getVariablesSpecFromEnvMap(jobSpec.CommandEnvVars, "commands") - runVariables := getVariablesSpecFromEnvMap(jobSpec.RunEnvVars, "run") + stateVariables := getVariablesSpecFromEnvMap(jobSpec.StateEnvVars, StageState) + commandVariables := getVariablesSpecFromEnvMap(jobSpec.CommandEnvVars, StageCommands) + runVariables := getVariablesSpecFromEnvMap(jobSpec.RunEnvVars, StageRun) - if err := findDuplicatesInStage(stateVariables, "state"); err != nil { + if err := findDuplicatesInStage(stateVariables, StageState); err != nil { return nil, err } - if err := findDuplicatesInStage(commandVariables, "commands"); err != nil { + if err := findDuplicatesInStage(commandVariables, StageCommands); err != nil { return nil, err } - if err := findDuplicatesInStage(runVariables, "run"); err != nil { + if err := findDuplicatesInStage(runVariables, StageRun); err != nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/services/spec.go(3 hunks)
🔇 Additional comments (1)
backend/services/spec.go (1)
Line range hint
63-85: LGTM! Clean implementation of stage-specific variable handling.The stage parameter is correctly propagated to both interpolated and non-interpolated variables while maintaining the original logic.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: https://app.greptile.com/review/github.
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
Hi! thanks for the contribution, generally looks good but there needs to be more changes made in the cli side specifically this function:
digger/libs/spec/variables_provider.go
Line 12 in 91358c8
it needs to filter the 3 stages of variables and return them to the calling function similar to what was done on the backend side
backend/services/spec.go
Outdated
| return variablesSpec | ||
| } | ||
|
|
||
| func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string) (error) { |
There was a problem hiding this comment.
can you please add a few quick tests to this function ?
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review, not the entire PR.
The latest changes have addressed some of the previously identified compilation issues, specifically fixing the function naming and type references in the duplicate checking logic. The code now properly uses checkDuplicatesInStage consistently and correctly references spec.VariableSpec types.
However, a critical architectural flaw remains unresolved: the code implements stage-specific duplicate checking but continues to maintain the global duplicate check that defeats the entire purpose of the feature. Lines 200-208 in backend/services/spec.go still contain the original logic that checks for duplicates across ALL variables regardless of stage, which directly contradicts the new stage-aware approach and prevents the legitimate use case of having the same variable name in different stages (like TF_LOG in both 'state' and 'commands' stages).
The changes integrate with the existing digger workflow system where variables are categorized into three stages: 'state', 'commands', and 'run'. The new Stage field in VariableSpec enables proper categorization, and the per-stage validation functions work correctly in isolation. However, the retention of the global check creates a logical inconsistency that renders the stage-specific improvements ineffective.
Confidence score: 2/5
- This PR still contains a critical logical flaw that prevents it from achieving its stated goal
- Score reflects the unresolved architectural issue where global duplicate checking contradicts stage-specific validation
- Pay close attention to backend/services/spec.go lines 200-208 where the conflicting global duplicate check remains active
2 files reviewed, 1 comment
backend/services/spec.go
Outdated
| return variablesSpec | ||
| } | ||
|
|
||
| func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string) (error) { |
There was a problem hiding this comment.
syntax: Remove extra parentheses in function signature
| func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string) (error) { | |
| func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string) error { |
This comment has been minimized.
This comment has been minimized.
![bismuthdev[bot]](https://avatars.githubusercontent.com/in/957724?s=60&v=4){kind=small}
backend/services/spec.go
Outdated
| func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string) (error) { | ||
| // Extract the names from VariableSpec | ||
| justNames := lo.Map(variablesSpec, func(item spec.VariableSpec, i int) string { | ||
| return item.Name | ||
| }) | ||
|
|
||
| // Group names by their occurrence | ||
| nameCounts := lo.CountValues(justNames) | ||
|
|
||
| // Filter names that occur more than once | ||
| duplicates := lo.Keys(lo.PickBy(nameCounts, func(name string, count int) bool { | ||
| return count > 1 | ||
| })) | ||
|
|
||
| if len(duplicates) > 0 { | ||
| return fmt.Errorf("duplicate variable names found in '%s' stage: %v", stage, strings.Join(duplicates, ", ")) | ||
| } | ||
|
|
||
| return nil // No duplicates found | ||
| } |
There was a problem hiding this comment.
The findDuplicatesInStage function in backend/services/spec.go returns an error if duplicate variable names are found, but it doesn't include the job ID in the error message. This makes debugging more difficult because when an error occurs, it's not immediately clear which job caused the problem.
The fix modifies the function signature to accept a jobId parameter and includes this ID in the error message. This change will make it easier to trace and debug issues related to duplicate variables in specific jobs.
The corresponding calls to this function in GetSpecFromJob will also need to be updated to pass the job ID.
| func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string) (error) { | |
| // Extract the names from VariableSpec | |
| justNames := lo.Map(variablesSpec, func(item spec.VariableSpec, i int) string { | |
| return item.Name | |
| }) | |
| // Group names by their occurrence | |
| nameCounts := lo.CountValues(justNames) | |
| // Filter names that occur more than once | |
| duplicates := lo.Keys(lo.PickBy(nameCounts, func(name string, count int) bool { | |
| return count > 1 | |
| })) | |
| if len(duplicates) > 0 { | |
| return fmt.Errorf("duplicate variable names found in '%s' stage: %v", stage, strings.Join(duplicates, ", ")) | |
| } | |
| return nil // No duplicates found | |
| } | |
| func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string, jobId string) (error) { | |
| // Extract the names from VariableSpec | |
| justNames := lo.Map(variablesSpec, func(item spec.VariableSpec, i int) string { | |
| return item.Name | |
| }) | |
| // Group names by their occurrence | |
| nameCounts := lo.CountValues(justNames) | |
| // Filter names that occur more than once | |
| duplicates := lo.Keys(lo.PickBy(nameCounts, func(name string, count int) bool { | |
| return count > 1 | |
| })) | |
| if len(duplicates) > 0 { | |
| return fmt.Errorf("duplicate variable names found in '%s' stage for job %s: %v", stage, jobId, strings.Join(duplicates, ", ")) | |
| } | |
| return nil // No duplicates found | |
| } |
There was a problem hiding this comment.
Hi @motatoes I have added the new spec_test.go file for tests requested for the function also modified provider file to filter 3 new variables.
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The most recent changes implement stage-aware variable handling to address issue #1795, which involved incorrect duplicate variable checking across different execution stages (state, commands, run). The key architectural change is in the VariablesProvider.GetVariables method in libs/spec/variables_provider.go, which now returns a nested map structure (map[string]map[string]string) instead of a flat map, grouping variables by their Stage field first before processing them.
The implementation groups variables using lo.GroupBy based on the variable.Stage field, then processes each stage independently while maintaining the same security and interpolation logic for secrets, interpolated values, and plain variables. This allows the same variable name to exist across different stages without conflict, which aligns with the intended digger.yml configuration behavior where variables can be scoped to specific stages (state, commands, run) and reused across stages with different values.
This change integrates with the backend services layer where duplicate checking has been modified to occur within individual stages rather than globally, and supports the CLI's need for stage-specific variable exposure during execution.
Confidence score: 3/5
- This PR introduces a significant breaking API change that will require updates to all calling code
- Score reflects the fundamental change in return type from flat to nested map structure without visible updates to dependent code
- Pay close attention to all files that call
GetVariablesmethod as they will need refactoring
2 files reviewed, 2 comments
| type VariablesProvider struct{} | ||
|
|
||
| func (p VariablesProvider) GetVariables(variables []VariableSpec) (map[string]string, error) { | ||
| func (p VariablesProvider) GetVariables(variables []VariableSpec) (map[string]map[string]string, error) { |
There was a problem hiding this comment.
logic: Breaking change: return type changed from map[string]string to map[string]map[string]string. All calling code must be updated to handle the new nested structure.
| if len(secrets) > 0 && private_key == "" { | ||
| return nil, fmt.Errorf("digger private key not supplied, unable to decrypt secrets") | ||
| } |
There was a problem hiding this comment.
style: Private key validation moved inside stage loop - this could result in duplicate error messages if multiple stages contain secrets. Consider moving this check outside the loop.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The latest changes in this PR focus on updating the test file libs/spec/variables_provider_test.go to accommodate the breaking change in the GetVariables method's return type from map[string]string to map[string]map[string]string. This modification supports the new stage-aware variable management system where variables are grouped by their stage (state, commands, run).
The test file updates include modifying test validation logic to access variables through the new nested structure variables[stage][variableName]. However, the test cases themselves have not been properly updated to initialize the Stage field on VariableSpec objects, which is critical for the new stage-based functionality.
Confidence score: 1/5
- This PR will cause immediate runtime failures due to missing Stage field initialization in test cases
- Score reflects critical implementation gaps that will cause panics when accessing
variables[stage]with empty stage keys - Pay close attention to
libs/spec/variables_provider_test.gowhich needs proper Stage field initialization in all test cases
1 file reviewed, no comments
This comment has been minimized.
This comment has been minimized.
| // Since the return type is map[string]map[string]string, we need to check the stage and variable name | ||
| stage := tc.variables[0].Stage | ||
| if _, ok := variables[stage]; !ok { | ||
| t.Errorf("Expected stage '%s' not found in variables map", stage) | ||
| } else { | ||
| value := variables[stage][tc.variables[0].Name] | ||
| assert.Equal(t, tc.plaintext, value) | ||
| } |
There was a problem hiding this comment.
The test verification logic in variables_provider_test.go only checks the first variable in the test case, which is insufficient for testing the stage grouping functionality.
When testing multiple variables across different stages, the current verification logic doesn't validate that all variables are correctly placed in their respective stage groups in the result map. This means that even if we add test cases with multiple variables in different stages, the test won't properly verify that the grouping functionality works as expected.
I've updated the verification logic to handle both single-variable and multi-variable test cases. For multi-variable test cases, it verifies that each variable is correctly placed in its respective stage group and has the expected value. This ensures that the stage grouping functionality in the GetVariables method is properly tested.
| // Merge variables for each stage into the job's environment variables | ||
| for _, vars := range variablesMap { | ||
| job.StateEnvVars = lo.Assign(job.StateEnvVars, vars) | ||
| job.CommandEnvVars = lo.Assign(job.CommandEnvVars, vars) | ||
| job.RunEnvVars = lo.Assign(job.RunEnvVars, vars) | ||
| } |
There was a problem hiding this comment.
The GetVariables function in libs/spec/variables_provider.go has been changed to return map[string]map[string]string instead of map[string]string, which is a breaking change. In cli/pkg/spec/spec.go, the code is incorrectly iterating through the outer map and applying all variables to all environment variable maps, regardless of their stage.
The correct approach is to access each stage's variables specifically and apply them to the corresponding environment variable map. The variables are now organized by stage (state, commands, run) and should be applied to their respective environment variable maps.
| // Merge variables for each stage into the job's environment variables | |
| for _, vars := range variablesMap { | |
| job.StateEnvVars = lo.Assign(job.StateEnvVars, vars) | |
| job.CommandEnvVars = lo.Assign(job.CommandEnvVars, vars) | |
| job.RunEnvVars = lo.Assign(job.RunEnvVars, vars) | |
| } | |
| // Merge variables for each stage into the job's environment variables | |
| // Each stage has its own set of variables | |
| if stateVars, ok := variablesMap["state"]; ok { | |
| job.StateEnvVars = lo.Assign(job.StateEnvVars, stateVars) | |
| } | |
| if commandVars, ok := variablesMap["commands"]; ok { | |
| job.CommandEnvVars = lo.Assign(job.CommandEnvVars, commandVars) | |
| } | |
| if runVars, ok := variablesMap["run"]; ok { | |
| job.RunEnvVars = lo.Assign(job.RunEnvVars, runVars) | |
| } |
| // Group variables by their stage | ||
| stagedVariables := lo.GroupBy(variables, func(variable VariableSpec) string { | ||
| return variable.Stage | ||
| }) |
There was a problem hiding this comment.
When a VariableSpec is created without a Stage value, it is grouped under an empty string key ("") in the stagedVariables map. This can lead to confusion and potential issues when accessing variables by stage.
The fix provides a default stage name ("default") for variables that don't have a stage specified, making the behavior more explicit and preventing empty string keys in the map.
| // Group variables by their stage | |
| stagedVariables := lo.GroupBy(variables, func(variable VariableSpec) string { | |
| return variable.Stage | |
| }) | |
| // Group variables by their stage | |
| stagedVariables := lo.GroupBy(variables, func(variable VariableSpec) string { | |
| if variable.Stage == "" { | |
| return "default" | |
| } | |
| return variable.Stage | |
| }) |
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The PR has been updated to implement stage-specific variable handling to solve issue #1795. The key changes since the last review include:
-
Fixed function naming and compilation issues: The
findDuplicatesInStagefunction has been corrected to properly handle the stage-specific duplicate checking. The function now uses the correct type references (spec.VariableSpec) and has been properly integrated with the calling code. -
Resolved breaking change in return type: The
GetVariablesmethod inVariablesProvidernow properly returnsmap[string]map[string]string(nested map by stage) instead of the previous flatmap[string]string. This enables proper stage-aware variable organization. -
Enhanced CLI variable assignment: The CLI code in
spec.gohas been updated to correctly handle the new nested variable structure. Variables are now properly assigned to their respective stage-specific environment maps (StateEnvVars,CommandEnvVars,RunEnvVars) instead of being applied to all stages indiscriminately. -
Comprehensive test coverage: New test functions have been added to validate the stage-aware functionality, including tests for success cases, error scenarios, and the new nested map structure returned by
GetVariables.
The implementation addresses the core issue where variables with the same name across different stages (state, commands, run) were incorrectly flagged as duplicates. Now, duplicate checking occurs within each stage individually, allowing legitimate same-named variables across different execution phases while preventing actual duplicates within each stage.
Confidence score: 2/5
- This PR introduces significant architectural changes that could break existing functionality if not thoroughly tested
- Score reflects the complexity of the breaking changes and the potential for runtime issues in production
- Pay close attention to the backend services files, particularly the duplicate checking logic and variable assignment code
Context used:
Context - Use consistent and descriptive keys for debug logging. For example, prefer 'prBatchCount' over 'len(prBatches)' to maintain clarity and adhere to naming conventions. (link)
Context - Enrich debug logs with relevant context. Always include identifiers like 'prNumber' in logs to facilitate easier tracing and troubleshooting. (link)
4 files reviewed, no comments
This comment has been minimized.
This comment has been minimized.
| // Since the return type is map[string]map[string]string, we need to check the stage and variable name | ||
| stage := tc.variables[0].Stage | ||
| if _, ok := variables[stage]; !ok { | ||
| t.Errorf("Expected stage '%s' not found in variables map", stage) | ||
| } else { | ||
| value := variables[stage][tc.variables[0].Name] | ||
| assert.Equal(t, tc.plaintext, value) | ||
| } |
There was a problem hiding this comment.
There is a bug in the variables_provider_test.go file. In the VariablesProvider.GetVariables method (in variables_provider.go), variables without a stage are assigned to a 'default' stage (line 17). However, in the test file, it's checking for the exact stage value without accounting for this default stage assignment.
This will cause test failures when variables with empty stage values are tested, as the test is looking for the variable under the empty stage key, but the implementation puts it under the "default" key.
The fix adds a check to use "default" as the stage name when the stage is empty, matching the behavior in the implementation.
| // Since the return type is map[string]map[string]string, we need to check the stage and variable name | |
| stage := tc.variables[0].Stage | |
| if _, ok := variables[stage]; !ok { | |
| t.Errorf("Expected stage '%s' not found in variables map", stage) | |
| } else { | |
| value := variables[stage][tc.variables[0].Name] | |
| assert.Equal(t, tc.plaintext, value) | |
| } | |
| // Since the return type is map[string]map[string]string, we need to check the stage and variable name | |
| stage := tc.variables[0].Stage | |
| if stage == "" { | |
| stage = "default" // Handle empty stage case | |
| } | |
| if _, ok := variables[stage]; !ok { | |
| t.Errorf("Expected stage '%s' not found in variables map", stage) | |
| } else { | |
| value := variables[stage][tc.variables[0].Name] | |
| assert.Equal(t, tc.plaintext, value) | |
| } |
| if err := findDuplicatesInStage(stateVariables, "state", jobId); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := findDuplicatesInStage(commandVariables, "commands", jobId); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := findDuplicatesInStage(runVariables, "run", jobId); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
There's an inconsistency in the code where a new variable jobId is created to store job.DiggerJobID, but job.DiggerJobID is still used directly in log messages throughout the function. For example, in lines 202 and 213, the code uses job.DiggerJobID in log messages instead of the local jobId variable. This inconsistency should be fixed by using the local jobId variable consistently throughout the function.
| if err := findDuplicatesInStage(stateVariables, "state", jobId); err != nil { | |
| return nil, err | |
| } | |
| if err := findDuplicatesInStage(commandVariables, "commands", jobId); err != nil { | |
| return nil, err | |
| } | |
| if err := findDuplicatesInStage(runVariables, "run", jobId); err != nil { | |
| return nil, err | |
| } | |
| if err := findDuplicatesInStage(stateVariables, "state", jobId); err != nil { | |
| return nil, err | |
| } | |
| if err := findDuplicatesInStage(commandVariables, "commands", jobId); err != nil { | |
| return nil, err | |
| } | |
| if err := findDuplicatesInStage(runVariables, "run", jobId); err != nil { | |
| return nil, err | |
| } |
| func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string, jobId string) (error) { | ||
| // Extract the names from VariableSpec | ||
| justNames := lo.Map(variablesSpec, func(item spec.VariableSpec, i int) string { | ||
| return item.Name | ||
| }) | ||
| // Group names by their occurrence | ||
| nameCounts := lo.CountValues(justNames) | ||
| // Filter names that occur more than once | ||
| duplicates := lo.Keys(lo.PickBy(nameCounts, func(name string, count int) bool { | ||
| return count > 1 | ||
| })) | ||
| if len(duplicates) > 0 { | ||
| return fmt.Errorf("duplicate variable names found in '%s' stage for job %s: %v", stage, jobId, strings.Join(duplicates, ", ")) | ||
| } | ||
| return nil // No duplicates found | ||
| } |
There was a problem hiding this comment.
The findDuplicatesInStage function doesn't handle the case when the stage parameter is empty. This could lead to a confusing error message where the stage is shown as an empty string.
The tests in spec_test.go don't explicitly test for this edge case, although they do pass an empty jobId parameter in several tests, which the function handles correctly.
The fix adds a check for an empty stage parameter and sets it to "unknown" in that case, which makes the error message more informative.
| func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string, jobId string) (error) { | |
| // Extract the names from VariableSpec | |
| justNames := lo.Map(variablesSpec, func(item spec.VariableSpec, i int) string { | |
| return item.Name | |
| }) | |
| // Group names by their occurrence | |
| nameCounts := lo.CountValues(justNames) | |
| // Filter names that occur more than once | |
| duplicates := lo.Keys(lo.PickBy(nameCounts, func(name string, count int) bool { | |
| return count > 1 | |
| })) | |
| if len(duplicates) > 0 { | |
| return fmt.Errorf("duplicate variable names found in '%s' stage for job %s: %v", stage, jobId, strings.Join(duplicates, ", ")) | |
| } | |
| return nil // No duplicates found | |
| } | |
| func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string, jobId string) (error) { | |
| // Handle empty stage parameter | |
| if stage == "" { | |
| stage = "unknown" | |
| } | |
| // Extract the names from VariableSpec | |
| justNames := lo.Map(variablesSpec, func(item spec.VariableSpec, i int) string { | |
| return item.Name | |
| }) | |
| // Group names by their occurrence | |
| nameCounts := lo.CountValues(justNames) | |
| // Filter names that occur more than once | |
| duplicates := lo.Keys(lo.PickBy(nameCounts, func(name string, count int) bool { | |
| return count > 1 | |
| })) | |
| if len(duplicates) > 0 { | |
| return fmt.Errorf("duplicate variable names found in '%s' stage for job %s: %v", stage, jobId, strings.Join(duplicates, ", ")) | |
| } | |
| return nil // No duplicates found | |
| } |
added stage field in spec and solved issue of checking duplicates in multiple stages
solves issue: #1795
Summary by CodeRabbit
New Features
Improvements