Skip to content
Open
Show file tree
Hide file tree
Changes from 6 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")
}
10 changes: 7 additions & 3 deletions cli/pkg/spec/spec.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,13 @@ func RunSpec(
reportError(spec, backendApi, msg, err)
usage.ReportErrorAndExit(spec.VCS.Actor, fmt.Sprintf("could not get variables from provider: %v", err), 1)
}
job.StateEnvVars = lo.Assign(job.StateEnvVars, variablesMap)
job.CommandEnvVars = lo.Assign(job.CommandEnvVars, variablesMap)
job.RunEnvVars = lo.Assign(job.RunEnvVars, variablesMap)

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

Choose a reason for hiding this comment

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

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.

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


jobs := []scheduler.Job{job}

Expand Down
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
}
15 changes: 12 additions & 3 deletions libs/spec/variables_provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ import (
"crypto/x509"
"encoding/base64"
"encoding/pem"
"github.com/stretchr/testify/assert"
"os"
"testing"

"github.com/stretchr/testify/assert"
)

// generateTestKeyPair generates a test RSA key pair
Expand Down Expand Up @@ -76,6 +77,7 @@ func TestDecryptProvider(t *testing.T) {
}
tc.variables[0].Value = base64.StdEncoding.EncodeToString(v)
}

variables, err := VariablesProvider{}.GetVariables(tc.variables)
if tc.expectError {
if err == nil {
Expand All @@ -85,8 +87,15 @@ func TestDecryptProvider(t *testing.T) {
if err != nil {
t.Errorf("Unexpected error: %v", err)
}
value := variables[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 _, 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)
}
Comment on lines +91 to +98
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +91 to +98
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

}
})
}
Expand Down