-
Notifications
You must be signed in to change notification settings - Fork 564
added stage field in spec and solved issue of checking duplicates of multip… #1875
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
base: develop
Are you sure you want to change the base?
Changes from all commits
47f3163
34e2b2a
318babd
d2518e7
0d23fe7
a82ef6c
955e681
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 | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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_") { | ||||||||||||||||||||||||||||||||||||||
|
@@ -134,31 +134,62 @@ 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, 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 GetSpecFromJob(job models.DiggerJob) (*spec.Spec, error) { | ||||||||||||||||||||||||||||||||||||||
var jobSpec scheduler.JobJson | ||||||||||||||||||||||||||||||||||||||
var jobId = job.DiggerJobID; | ||||||||||||||||||||||||||||||||||||||
err := json.Unmarshal([]byte(job.SerializedJobSpec), &jobSpec) | ||||||||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||||||||
slog.Error("Could not unmarshal job spec", "jobId", job.DiggerJobID, "error", err) | ||||||||||||||||||||||||||||||||||||||
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", 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 | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+182
to
+190
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's an inconsistency in the code where a new variable
Suggested change
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
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...) | ||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
// filepath: digger/backend/services/spec_test.go | ||
package services | ||
|
||
import ( | ||
"os" | ||
"testing" | ||
|
||
"github.com/diggerhq/digger/libs/spec" | ||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
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") | ||
} | ||
|
||
// TestGetVariables_Success tests the GetVariables function for successful variable retrieval. | ||
func TestGetVariables_Success(t *testing.T) { | ||
variablesSpec := []spec.VariableSpec{ | ||
{Name: "VAR1", Value: "value1", Stage: "stage1"}, | ||
{Name: "VAR2", Value: "value2", Stage: "stage1"}, | ||
{Name: "VAR3", Value: "value3", Stage: "stage2"}, | ||
} | ||
|
||
variablesProvider := spec.VariablesProvider{} | ||
variablesMap, err := variablesProvider.GetVariables(variablesSpec) | ||
|
||
assert.Nil(t, err, "Expected no error when retrieving variables") | ||
assert.Equal(t, "value1", variablesMap["stage1"]["VAR1"]) | ||
assert.Equal(t, "value2", variablesMap["stage1"]["VAR2"]) | ||
assert.Equal(t, "value3", variablesMap["stage2"]["VAR3"]) | ||
} | ||
|
||
// TestGetVariables_MissingPrivateKey tests the GetVariables function when a private key is required but missing. | ||
func TestGetVariables_MissingPrivateKey(t *testing.T) { | ||
variablesSpec := []spec.VariableSpec{ | ||
{Name: "VAR1", Value: "encryptedValue", IsSecret: true, Stage: "stage1"}, | ||
} | ||
|
||
variablesProvider := spec.VariablesProvider{} | ||
_, err := variablesProvider.GetVariables(variablesSpec) | ||
|
||
assert.NotNil(t, err, "Expected an error when private key is missing") | ||
assert.Contains(t, err.Error(), "digger private key not supplied, unable to decrypt secrets") | ||
} | ||
|
||
// TestGetVariables_DecryptError tests the GetVariables function when decryption fails. | ||
func TestGetVariables_DecryptError(t *testing.T) { | ||
os.Setenv("DIGGER_PRIVATE_KEY", "invalidPrivateKey") | ||
defer os.Unsetenv("DIGGER_PRIVATE_KEY") | ||
|
||
variablesSpec := []spec.VariableSpec{ | ||
{Name: "VAR1", Value: "encryptedValue", IsSecret: true, Stage: "stage1"}, | ||
} | ||
|
||
variablesProvider := spec.VariablesProvider{} | ||
_, err := variablesProvider.GetVariables(variablesSpec) | ||
|
||
assert.NotNil(t, err, "Expected an error when decryption fails") | ||
assert.Contains(t, err.Error(), "could not decrypt value using private key") | ||
} | ||
|
||
// TestGetVariables_InterpolatedVariables tests the GetVariables function for interpolated variables. | ||
func TestGetVariables_InterpolatedVariables(t *testing.T) { | ||
os.Setenv("INTERPOLATED_VAR", "interpolatedValue") | ||
defer os.Unsetenv("INTERPOLATED_VAR") | ||
|
||
variablesSpec := []spec.VariableSpec{ | ||
{Name: "VAR1", Value: "INTERPOLATED_VAR", IsInterpolated: true, Stage: "stage1"}, | ||
} | ||
|
||
variablesProvider := spec.VariablesProvider{} | ||
variablesMap, err := variablesProvider.GetVariables(variablesSpec) | ||
|
||
assert.Nil(t, err, "Expected no error when retrieving interpolated variables") | ||
assert.Equal(t, "interpolatedValue", variablesMap["stage1"]["VAR1"]) | ||
} | ||
|
||
// TestGetVariables_EmptyVariables tests the GetVariables function with an empty variables spec. | ||
func TestGetVariables_EmptyVariables(t *testing.T) { | ||
variablesSpec := []spec.VariableSpec{} | ||
|
||
variablesProvider := spec.VariablesProvider{} | ||
variablesMap, err := variablesProvider.GetVariables(variablesSpec) | ||
|
||
assert.Nil(t, err, "Expected no error when variablesSpec is empty") | ||
assert.Empty(t, variablesMap, "Expected variablesMap to be empty") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,31 +9,60 @@ 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) { | ||
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. 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 { | ||
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
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. 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 | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||||||||||||||||||||||||||||||
|
@@ -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 { | ||||||||||||||||||||||||||||||||||||||||
|
@@ -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
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. The test verification logic in 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
Comment on lines
+91
to
+98
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 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
|
||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
|
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.
The
findDuplicatesInStage
function doesn't handle the case when thestage
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 emptyjobId
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.