Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 38 additions & 4 deletions backend/services/spec.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ func GetRunNameFromJob(job models.DiggerJob) (*string, error) {
return &runName, nil
}

func getVariablesSpecFromEnvMap(envVars map[string]string) []spec.VariableSpec {
func getVariablesSpecFromEnvMap(envVars map[string]string, stage string) []spec.VariableSpec {
variablesSpec := make([]spec.VariableSpec, 0)
for k, v := range envVars {
if strings.HasPrefix(v, "$DIGGER_") {
Expand All @@ -134,19 +134,42 @@ func getVariablesSpecFromEnvMap(envVars map[string]string) []spec.VariableSpec {
Value: val,
IsSecret: false,
IsInterpolated: true,
Stage: stage,
})
} else {
variablesSpec = append(variablesSpec, spec.VariableSpec{
Name: k,
Value: v,
IsSecret: false,
IsInterpolated: false,
Stage: stage,
})
}
}
return variablesSpec
}

func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string) (error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please add a few quick tests to this function ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax: Remove extra parentheses in function signature

Suggested change
func findDuplicatesInStage(variablesSpec []spec.VariableSpec, stage string) (error) {
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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


func GetSpecFromJob(job models.DiggerJob) (*spec.Spec, error) {
var jobSpec scheduler.JobJson
err := json.Unmarshal([]byte(job.SerializedJobSpec), &jobSpec)
Expand All @@ -155,10 +178,21 @@ func GetSpecFromJob(job models.DiggerJob) (*spec.Spec, error) {
return nil, fmt.Errorf("could not marshal json string: %v", err)
}

stateVariables := getVariablesSpecFromEnvMap(jobSpec.StateEnvVars, "state")
commandVariables := getVariablesSpecFromEnvMap(jobSpec.CommandEnvVars, "commands")
runVariables := getVariablesSpecFromEnvMap(jobSpec.RunEnvVars, "run")

if err := findDuplicatesInStage(stateVariables, "state"); err != nil {
return nil, err
}
if err := findDuplicatesInStage(commandVariables, "commands"); err != nil {
return nil, err
}
if err := findDuplicatesInStage(runVariables, "run"); err != nil {
return nil, err
}

variablesSpec := make([]spec.VariableSpec, 0)
stateVariables := getVariablesSpecFromEnvMap(jobSpec.StateEnvVars)
commandVariables := getVariablesSpecFromEnvMap(jobSpec.CommandEnvVars)
runVariables := getVariablesSpecFromEnvMap(jobSpec.RunEnvVars)
variablesSpec = append(variablesSpec, stateVariables...)
variablesSpec = append(variablesSpec, commandVariables...)
variablesSpec = append(variablesSpec, runVariables...)
Expand Down
44 changes: 44 additions & 0 deletions backend/services/spec_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// filepath: digger/backend/services/spec_test.go
package services

import (
// "fmt"
"github.com/diggerhq/digger/libs/spec"
"github.com/stretchr/testify/assert"
"testing"
)

func TestFindDuplicatesInStage_NoDuplicates(t *testing.T) {
variablesSpec := []spec.VariableSpec{
{Name: "VAR1"},
{Name: "VAR2"},
{Name: "VAR3"},
}
err := findDuplicatesInStage(variablesSpec, "test-stage")
assert.Nil(t, err, "Expected no error when there are no duplicates")
}

func TestFindDuplicatesInStage_WithDuplicates(t *testing.T) {
variablesSpec := []spec.VariableSpec{
{Name: "VAR1"},
{Name: "VAR2"},
{Name: "VAR1"},
}
err := findDuplicatesInStage(variablesSpec, "test-stage")
assert.NotNil(t, err, "Expected an error when duplicates are present")
assert.Contains(t, err.Error(), "duplicate variable names found in 'test-stage' stage: VAR1")
}

func TestFindDuplicatesInStage_EmptyVariablesSpec(t *testing.T) {
variablesSpec := []spec.VariableSpec{}
err := findDuplicatesInStage(variablesSpec, "test-stage")
assert.Nil(t, err, "Expected no error when variablesSpec is empty")
}

func TestFindDuplicatesInStage_SingleVariable(t *testing.T) {
variablesSpec := []spec.VariableSpec{
{Name: "VAR1"},
}
err := findDuplicatesInStage(variablesSpec, "test-stage")
assert.Nil(t, err, "Expected no error when there is only one variable")
}
1 change: 1 addition & 0 deletions libs/spec/models.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ type VariableSpec struct {
Value string `json:"value"`
IsSecret bool `json:"is_secret"`
IsInterpolated bool `json:"is_interpolated"`
Stage string `json:"stage"`
}

type SpecType string
Expand Down
58 changes: 42 additions & 16 deletions libs/spec/variables_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,31 +9,57 @@ import (

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private_key := os.Getenv("DIGGER_PRIVATE_KEY")
secrets := lo.Filter(variables, func(variable VariableSpec, i int) bool {
return variable.IsSecret

// Group variables by their stage
stagedVariables := lo.GroupBy(variables, func(variable VariableSpec) string {
return variable.Stage
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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
})

if len(secrets) > 0 && private_key == "" {
return nil, fmt.Errorf("digger private key not supplied, unable to decrypt secrets")
}

res := make(map[string]string)
result := make(map[string]map[string]string)

for stage, vars := range stagedVariables {
stageResult := make(map[string]string)

// Filter variables into three categories
secrets := lo.Filter(vars, func(variable VariableSpec, i int) bool {
return variable.IsSecret
})
interpolated := lo.Filter(vars, func(variable VariableSpec, i int) bool {
return variable.IsInterpolated
})
plain := lo.Filter(vars, func(variable VariableSpec, i int) bool {
return !variable.IsSecret && !variable.IsInterpolated
})

// Check if private key is required for secrets
if len(secrets) > 0 && private_key == "" {
return nil, fmt.Errorf("digger private key not supplied, unable to decrypt secrets")
}
Comment on lines +40 to +42
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


for _, v := range variables {
if v.IsSecret {
// Process secret variables
for _, v := range secrets {
value, err := digger_crypto.DecryptValueUsingPrivateKey(v.Value, private_key)
if err != nil {
return nil, fmt.Errorf("could not decrypt value using private key: %v", err)
}
res[v.Name] = string(value)
} else if v.IsInterpolated {
// if it is an interpolated value we get it form env variable of the variable
res[v.Name] = os.Getenv(v.Value)
} else {
res[v.Name] = v.Value
stageResult[v.Name] = string(value)
}

// Process interpolated variables
for _, v := range interpolated {
stageResult[v.Name] = os.Getenv(v.Value)
}

// Process plain variables
for _, v := range plain {
stageResult[v.Name] = v.Value
}

// Add the processed variables for the current stage to the result
result[stage] = stageResult
}

return res, nil
return result, nil
}