Skip to content

Commit

Permalink
respect types for config set in the yaml file
Browse files Browse the repository at this point in the history
The yaml language provider currently gets the config via RPC and then
using some magic to try and infer the type of the variable it gets as
a string over the RPC interface.  However in many cases that's not
quite what the user wants, e.g. someone might have an AWS account ID
that starts with a `0`.  If that gets interpreted as a number the
account ID will no longer be correct.

Prefer the config nodes we parse out of the `Pulumi.yaml` file over
the ones we get through the RPC, falling back to the RPC ones when we
don't have more information in the yaml.
  • Loading branch information
tgummerer committed Oct 10, 2023
1 parent 5a62b63 commit e234583
Show file tree
Hide file tree
Showing 7 changed files with 77 additions and 31 deletions.
54 changes: 39 additions & 15 deletions pkg/pulumiyaml/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -731,29 +731,45 @@ func (r *Runner) Evaluate(ctx *pulumi.Context) syntax.Diagnostics {
return r.Run(programEvaluator{evalContext: eCtx, pulumiCtx: ctx})
}

func getPulumiConfNodes(config map[string]string) ([]configNode, error) {
func findConfigEntry(key string, entries []ast.ConfigMapEntry) *ast.ConfigMapEntry {
for _, entry := range entries {
if entry.Key.GetValue() == key {
return &entry
}
}
return nil
}

func getPulumiConfNodes(config map[string]string, astNodes []ast.ConfigMapEntry) ([]configNode, error) {
nodes := make([]configNode, len(config))
var errors multierror.Error
var errs multierror.Error
idx := 0
for k, v := range config {
// We default types to strings to avoid error cascades on mis-typed values.
typ := ctypes.String
var value interface{} = v
if v, t, err := getConfigNode(v); err == nil {
typ = t
value = v
// Check if we can find the type of the config entry in the AST
e := findConfigEntry(k, astNodes)
if e != nil {
nodes[idx] = configNodeYaml(*e)
idx++
} else {
errors.Errors = append(errors.Errors, err)
}
n := configNodeEnv{
Key: k,
Value: value,
Type: typ,
if v, t, err := getConfigNode(v); err == nil {
typ = t
value = v
} else {
errs.Errors = append(errs.Errors, err)
}
n := configNodeEnv{
Key: k,
Value: value,
Type: typ,
}
nodes[idx] = n
idx++
}
nodes[idx] = n
idx++
}
return nodes, errors.ErrorOrNil()
return nodes, errs.ErrorOrNil()
}

// setIntermediates is called for convert and runtime evaluation
Expand All @@ -766,7 +782,8 @@ func (r *Runner) setIntermediates(config map[string]string, force bool) {
}

r.intermediates = []graphNode{}
confNodes, err := getPulumiConfNodes(config)
configAstEntries := append(r.t.Config.Entries, r.t.Configuration.Entries...)
confNodes, err := getPulumiConfNodes(config, configAstEntries)
if err != nil && !force {
r.sdiags.Extend(syntax.Error(nil, err.Error(), ""))
return
Expand Down Expand Up @@ -928,6 +945,13 @@ func (e *programEvaluator) registerConfig(intm configNode) (interface{}, bool) {
c.Type.Value, ctypes.ConfigTypes)
}

// Expressions will always return Numbers and
// not Ints, but we're happy with ints as
// well.
if expectedType == ctypes.Number && t == ctypes.Int {
expectedType = ctypes.Int
}

// We have both a default value and a explicit type. Make sure they
// agree.
if ctypes.IsValidType(expectedType) && t != expectedType {
Expand Down
3 changes: 2 additions & 1 deletion pkg/pulumiyaml/run_plugin_version_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"testing"

"github.com/hexops/autogold"
"github.com/pulumi/pulumi-yaml/pkg/pulumiyaml/ast"
"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -164,7 +165,7 @@ outputs:
})
wantPlugins.Equal(t, gotPlugins)

confNodes, err := getPulumiConfNodes(nil)
confNodes, err := getPulumiConfNodes(nil, []ast.ConfigMapEntry{})
assert.Nil(t, err)
_, diags = topologicallySortedResources(tmpl, confNodes)
requireNoErrors(t, tmpl, diags)
Expand Down
30 changes: 30 additions & 0 deletions pkg/pulumiyaml/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,36 @@ variables:
})
}

func TestConfigType(t *testing.T) {
t.Parallel()
const text = `name: test-yaml
runtime: yaml
configuration:
test-var:
type: string
default: "default"
`
tmpl := yamlTemplate(t, text)
testTemplate(t, tmpl, func(e *programEvaluator) {
assert.Equal(t, "default", e.config["test-var"])
})
}

func TestConfigMismatchedTypes(t *testing.T) {
t.Parallel()
const text = `name: test-yaml
runtime: yaml
configuration:
test-var:
type: string
default: 42
`
tmpl := yamlTemplate(t, text)
diags := testTemplateDiags(t, tmpl, func(e *programEvaluator) {})
require.True(t, diags.HasErrors())
assert.Equal(t, "<stdin>:4:3: type mismatch: default value of type number but type string was specified", diagString(diags[0]))
}

func TestPropertiesAbsent(t *testing.T) {
t.Parallel()

Expand Down
9 changes: 0 additions & 9 deletions pkg/pulumiyaml/sort.go
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,6 @@ func checkUniqueNode(intermediates map[string]graphNode, node graphNode) syntax.
}

if other, found := intermediates[name]; found {
// if duplicate key from config/ configuration, do not warn about using configuration again
if isConfigNodeEnv(node) || isConfigNodeEnv(other) {
return diags
}
if node.valueKind() == other.valueKind() {
diags.Extend(ast.ExprError(key, fmt.Sprintf("found duplicate %s %s", node.valueKind(), name), ""))
} else {
Expand All @@ -244,11 +240,6 @@ func checkUniqueNode(intermediates map[string]graphNode, node graphNode) syntax.
return diags
}

func isConfigNodeEnv(n graphNode) bool {
_, ok := n.(configNodeEnv)
return ok
}

func stripConfigNamespace(n, s string) string {
return strings.TrimPrefix(s, n+":")
}
6 changes: 3 additions & 3 deletions pkg/pulumiyaml/sort_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func TestSortOrdered(t *testing.T) {
},
},
})
confNodes, err := getPulumiConfNodes(nil)
confNodes, err := getPulumiConfNodes(nil, []ast.ConfigMapEntry{})
assert.Nil(t, err)
resources, diags := topologicallySortedResources(tmpl, confNodes)
requireNoErrors(t, tmpl, diags)
Expand Down Expand Up @@ -102,7 +102,7 @@ func TestSortUnordered(t *testing.T) {
},
},
})
confNodes, err := getPulumiConfNodes(nil)
confNodes, err := getPulumiConfNodes(nil, []ast.ConfigMapEntry{})
assert.Nil(t, err)
resources, diags := topologicallySortedResources(tmpl, confNodes)
requireNoErrors(t, tmpl, diags)
Expand Down Expand Up @@ -133,7 +133,7 @@ func TestSortErrorCycle(t *testing.T) {
},
},
})
confNodes, err := getPulumiConfNodes(nil)
confNodes, err := getPulumiConfNodes(nil, []ast.ConfigMapEntry{})
assert.Nil(t, err)
_, err = topologicallySortedResources(tmpl, confNodes)
assert.Error(t, err)
Expand Down
4 changes: 2 additions & 2 deletions pkg/tests/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ func TestTypeCheckError(t *testing.T) {
}

//nolint:paralleltest // uses parallel programtest
func TestMismatchedConfigType(t *testing.T) {
func TestDuplicateConfig(t *testing.T) {
testWrapper(t, integrationDir("mismatched-config-type"), ExpectFailure, StderrValidator{
f: func(t *testing.T, stderr string) {
assert.Contains(t, stderr,
`config key "foo" cannot have conflicting types boolean, integer`)
`found duplicate config foo;`)
},
})
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/tests/testdata/mismatched-config-type/Pulumi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@ config:
foo: 3
configuration:
foo:
type: bool
type: boolean
default: true

0 comments on commit e234583

Please sign in to comment.