Skip to content
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

Added support for complex variables #1467

Merged
merged 16 commits into from
Jun 26, 2024
Merged
Show file tree
Hide file tree
Changes from 10 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
32 changes: 11 additions & 21 deletions bundle/config/mutator/resolve_resource_references_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func TestResolveClusterReference(t *testing.T) {
},
},
"some-variable": {
Value: &justString,
Value: justString,
},
},
},
Expand All @@ -53,8 +53,8 @@ func TestResolveClusterReference(t *testing.T) {

diags := bundle.Apply(context.Background(), b, ResolveResourceReferences())
require.NoError(t, diags.Error())
require.Equal(t, "1234-5678-abcd", *b.Config.Variables["my-cluster-id-1"].Value)
require.Equal(t, "9876-5432-xywz", *b.Config.Variables["my-cluster-id-2"].Value)
require.Equal(t, "1234-5678-abcd", b.Config.Variables["my-cluster-id-1"].Value)
require.Equal(t, "9876-5432-xywz", b.Config.Variables["my-cluster-id-2"].Value)
}

func TestResolveNonExistentClusterReference(t *testing.T) {
Expand All @@ -69,7 +69,7 @@ func TestResolveNonExistentClusterReference(t *testing.T) {
},
},
"some-variable": {
Value: &justString,
Value: justString,
},
},
},
Expand Down Expand Up @@ -105,7 +105,7 @@ func TestNoLookupIfVariableIsSet(t *testing.T) {

diags := bundle.Apply(context.Background(), b, ResolveResourceReferences())
require.NoError(t, diags.Error())
require.Equal(t, "random value", *b.Config.Variables["my-cluster-id"].Value)
require.Equal(t, "random value", b.Config.Variables["my-cluster-id"].Value)
}

func TestResolveServicePrincipal(t *testing.T) {
Expand All @@ -132,22 +132,19 @@ func TestResolveServicePrincipal(t *testing.T) {

diags := bundle.Apply(context.Background(), b, ResolveResourceReferences())
require.NoError(t, diags.Error())
require.Equal(t, "app-1234", *b.Config.Variables["my-sp"].Value)
require.Equal(t, "app-1234", b.Config.Variables["my-sp"].Value)
}

func TestResolveVariableReferencesInVariableLookups(t *testing.T) {
s := func(s string) *string {
return &s
}

s := "bar"
b := &bundle.Bundle{
Config: config.Root{
Bundle: config.Bundle{
Target: "dev",
},
Variables: map[string]*variable.Variable{
"foo": {
Value: s("bar"),
Value: s,
},
"lookup": {
Lookup: &variable.Lookup{
Expand All @@ -168,7 +165,7 @@ func TestResolveVariableReferencesInVariableLookups(t *testing.T) {
diags := bundle.Apply(context.Background(), b, bundle.Seq(ResolveVariableReferencesInLookup(), ResolveResourceReferences()))
require.NoError(t, diags.Error())
require.Equal(t, "cluster-bar-dev", b.Config.Variables["lookup"].Lookup.Cluster)
require.Equal(t, "1234-5678-abcd", *b.Config.Variables["lookup"].Value)
require.Equal(t, "1234-5678-abcd", b.Config.Variables["lookup"].Value)
}

func TestResolveLookupVariableReferencesInVariableLookups(t *testing.T) {
Expand Down Expand Up @@ -197,22 +194,15 @@ func TestResolveLookupVariableReferencesInVariableLookups(t *testing.T) {
}

func TestNoResolveLookupIfVariableSetWithEnvVariable(t *testing.T) {
s := func(s string) *string {
return &s
}

b := &bundle.Bundle{
Config: config.Root{
Bundle: config.Bundle{
Target: "dev",
},
Variables: map[string]*variable.Variable{
"foo": {
Value: s("bar"),
},
"lookup": {
Lookup: &variable.Lookup{
Cluster: "cluster-${var.foo}-${bundle.target}",
Cluster: "cluster-${bundle.target}",
},
},
},
Expand All @@ -227,5 +217,5 @@ func TestNoResolveLookupIfVariableSetWithEnvVariable(t *testing.T) {

diags := bundle.Apply(ctx, b, bundle.Seq(SetVariables(), ResolveVariableReferencesInLookup(), ResolveResourceReferences()))
require.NoError(t, diags.Error())
require.Equal(t, "1234-5678-abcd", *b.Config.Variables["lookup"].Value)
require.Equal(t, "1234-5678-abcd", b.Config.Variables["lookup"].Value)
}
8 changes: 8 additions & 0 deletions bundle/config/mutator/resolve_variable_references.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ func ResolveVariableReferencesInLookup() bundle.Mutator {
}, pattern: dyn.NewPattern(dyn.Key("variables"), dyn.AnyKey(), dyn.Key("lookup")), lookupFn: lookupForVariables}
}

func ResolveVariableReferencesInComplexVariables() bundle.Mutator {
andrewnester marked this conversation as resolved.
Show resolved Hide resolved
return &resolveVariableReferences{prefixes: []string{
"bundle",
"workspace",
"variables",
}, pattern: dyn.NewPattern(dyn.Key("variables"), dyn.AnyKey(), dyn.Key("value")), lookupFn: lookupForVariables}
}

func lookup(v dyn.Value, path dyn.Path) (dyn.Value, error) {
// Future opportunity: if we lookup this path in both the given root
// and the synthesized root, we know if it was explicitly set or implied to be empty.
Expand Down
6 changes: 1 addition & 5 deletions bundle/config/mutator/resolve_variable_references_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,6 @@ func TestResolveVariableReferences(t *testing.T) {
}

func TestResolveVariableReferencesToBundleVariables(t *testing.T) {
s := func(s string) *string {
return &s
}

b := &bundle.Bundle{
Config: config.Root{
Bundle: config.Bundle{
Expand All @@ -57,7 +53,7 @@ func TestResolveVariableReferencesToBundleVariables(t *testing.T) {
},
Variables: map[string]*variable.Variable{
"foo": {
Value: s("bar"),
Value: "bar",
},
},
},
Expand Down
8 changes: 6 additions & 2 deletions bundle/config/mutator/set_variables.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ func setVariable(ctx context.Context, v *variable.Variable, name string) diag.Di
// case: read and set variable value from process environment
envVarName := bundleVarPrefix + name
if val, ok := env.Lookup(ctx, envVarName); ok {
if v.IsComplex() {
return diag.Errorf(`setting via environment variables (%s) is not supported for complex variable %s`, envVarName, name)
}

err := v.Set(val)
if err != nil {
return diag.Errorf(`failed to assign value "%s" to variable %s from environment variable %s with error: %v`, val, name, envVarName, err)
Expand All @@ -45,9 +49,9 @@ func setVariable(ctx context.Context, v *variable.Variable, name string) diag.Di

// case: Set the variable to its default value
if v.HasDefault() {
err := v.Set(*v.Default)
err := v.Set(v.Default)
if err != nil {
return diag.Errorf(`failed to assign default value from config "%s" to variable %s with error: %v`, *v.Default, name, err)
return diag.Errorf(`failed to assign default value from config "%s" to variable %s with error: %v`, v.Default, name, err)
}
return nil
}
Expand Down
32 changes: 16 additions & 16 deletions bundle/config/mutator/set_variables_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,52 +15,52 @@ func TestSetVariableFromProcessEnvVar(t *testing.T) {
defaultVal := "default"
variable := variable.Variable{
Description: "a test variable",
Default: &defaultVal,
Default: defaultVal,
}

// set value for variable as an environment variable
t.Setenv("BUNDLE_VAR_foo", "process-env")

diags := setVariable(context.Background(), &variable, "foo")
require.NoError(t, diags.Error())
assert.Equal(t, *variable.Value, "process-env")
assert.Equal(t, variable.Value, "process-env")
}

func TestSetVariableUsingDefaultValue(t *testing.T) {
defaultVal := "default"
variable := variable.Variable{
Description: "a test variable",
Default: &defaultVal,
Default: defaultVal,
}

diags := setVariable(context.Background(), &variable, "foo")
require.NoError(t, diags.Error())
assert.Equal(t, *variable.Value, "default")
assert.Equal(t, variable.Value, "default")
}

func TestSetVariableWhenAlreadyAValueIsAssigned(t *testing.T) {
defaultVal := "default"
val := "assigned-value"
variable := variable.Variable{
Description: "a test variable",
Default: &defaultVal,
Value: &val,
Default: defaultVal,
Value: val,
}

// since a value is already assigned to the variable, it would not be overridden
// by the default value
diags := setVariable(context.Background(), &variable, "foo")
require.NoError(t, diags.Error())
assert.Equal(t, *variable.Value, "assigned-value")
assert.Equal(t, variable.Value, "assigned-value")
}

func TestSetVariableEnvVarValueDoesNotOverridePresetValue(t *testing.T) {
defaultVal := "default"
val := "assigned-value"
variable := variable.Variable{
Description: "a test variable",
Default: &defaultVal,
Value: &val,
Default: defaultVal,
Value: val,
}

// set value for variable as an environment variable
Expand All @@ -70,7 +70,7 @@ func TestSetVariableEnvVarValueDoesNotOverridePresetValue(t *testing.T) {
// by the value from environment
diags := setVariable(context.Background(), &variable, "foo")
require.NoError(t, diags.Error())
assert.Equal(t, *variable.Value, "assigned-value")
assert.Equal(t, variable.Value, "assigned-value")
}

func TestSetVariablesErrorsIfAValueCouldNotBeResolved(t *testing.T) {
Expand All @@ -92,15 +92,15 @@ func TestSetVariablesMutator(t *testing.T) {
Variables: map[string]*variable.Variable{
"a": {
Description: "resolved to default value",
Default: &defaultValForA,
Default: defaultValForA,
},
"b": {
Description: "resolved from environment vairables",
Default: &defaultValForB,
Default: defaultValForB,
},
"c": {
Description: "has already been assigned a value",
Value: &valForC,
Value: valForC,
},
},
},
Expand All @@ -110,7 +110,7 @@ func TestSetVariablesMutator(t *testing.T) {

diags := bundle.Apply(context.Background(), b, SetVariables())
require.NoError(t, diags.Error())
assert.Equal(t, "default-a", *b.Config.Variables["a"].Value)
assert.Equal(t, "env-var-b", *b.Config.Variables["b"].Value)
assert.Equal(t, "assigned-val-c", *b.Config.Variables["c"].Value)
assert.Equal(t, "default-a", b.Config.Variables["a"].Value)
assert.Equal(t, "env-var-b", b.Config.Variables["b"].Value)
assert.Equal(t, "assigned-val-c", b.Config.Variables["c"].Value)
}
22 changes: 21 additions & 1 deletion bundle/config/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,11 @@ func (r *Root) InitializeVariables(vars []string) error {
if _, ok := r.Variables[name]; !ok {
return fmt.Errorf("variable %s has not been defined", name)
}

if r.Variables[name].IsComplex() {
return fmt.Errorf("setting variables of complex type via --var flag is not supported: %s", name)
}

err := r.Variables[name].Set(val)
if err != nil {
return fmt.Errorf("failed to assign %s to %s: %s", val, name, err)
Expand Down Expand Up @@ -419,7 +424,7 @@ func rewriteShorthands(v dyn.Value) (dyn.Value, error) {
}

// For each variable, normalize its contents if it is a single string.
return dyn.Map(target, "variables", dyn.Foreach(func(_ dyn.Path, variable dyn.Value) (dyn.Value, error) {
return dyn.Map(target, "variables", dyn.Foreach(func(p dyn.Path, variable dyn.Value) (dyn.Value, error) {
switch variable.Kind() {

case dyn.KindString, dyn.KindBool, dyn.KindFloat, dyn.KindInt:
Expand All @@ -430,6 +435,21 @@ func rewriteShorthands(v dyn.Value) (dyn.Value, error) {
"default": variable,
}, variable.Location()), nil

case dyn.KindMap, dyn.KindSequence:
// Check if the original definition of variable has a type field.
typeV, err := dyn.GetByPath(v, p.Append(dyn.Key("type")))
if err != nil {
return variable, nil
}

if typeV.MustString() == "complex" {
return dyn.NewValue(map[string]dyn.Value{
"default": variable,
}, variable.Location()), nil
}

return variable, nil

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the PR include test coverage for this, i.e. both ways of setting the variable, as well as defining a top level complex variable and then specifying a different type in the override?

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 TAL at this?

default:
return variable, nil
}
Expand Down
8 changes: 4 additions & 4 deletions bundle/config/root_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func TestInitializeVariables(t *testing.T) {
root := &Root{
Variables: map[string]*variable.Variable{
"foo": {
Default: &fooDefault,
Default: fooDefault,
Description: "an optional variable since default is defined",
},
"bar": {
Expand All @@ -62,8 +62,8 @@ func TestInitializeVariables(t *testing.T) {

err := root.InitializeVariables([]string{"foo=123", "bar=456"})
assert.NoError(t, err)
assert.Equal(t, "123", *(root.Variables["foo"].Value))
assert.Equal(t, "456", *(root.Variables["bar"].Value))
assert.Equal(t, "123", (root.Variables["foo"].Value))
assert.Equal(t, "456", (root.Variables["bar"].Value))
}

func TestInitializeVariablesWithAnEqualSignInValue(t *testing.T) {
Expand All @@ -77,7 +77,7 @@ func TestInitializeVariablesWithAnEqualSignInValue(t *testing.T) {

err := root.InitializeVariables([]string{"foo=123=567"})
assert.NoError(t, err)
assert.Equal(t, "123=567", *(root.Variables["foo"].Value))
assert.Equal(t, "123=567", (root.Variables["foo"].Value))
}

func TestInitializeVariablesInvalidFormat(t *testing.T) {
Expand Down
Loading
Loading