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.
added stage field in spec and solved issue of checking duplicates in multiple stages
solves issue: #1795
Summary by CodeRabbit
New Features
Improvements