search

pulumi / pulumi-yaml

YAML language provider for Pulumi
Apache License 2.0
38 stars 11 forks source link

respect types for config set in the yaml file #509

Closed tgummerer closed 9 months ago

tgummerer commented 9 months ago

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.

This feels a little bit awkward still, and maybe we should be passing the types through the RPC? Curious to hear peoples opinions about this.

Fixes #453

Frassle commented 9 months ago

This feels a little bit awkward still, and maybe we should be passing the types through the RPC? Curious to hear peoples opinions about this.

We probably should. All the other languages have to deal with just the RPC protocol, they don't (and shouldn't) look at the config yaml files. Plus with esc and project config the RPC protocol has far better information than the yaml plugin can hope to keep up with.

tgummerer commented 9 months ago

I think this is ready for another review, now taking the types from the RPC call. Before merging this it needs https://github.com/pulumi/pulumi/pull/14244, which is also required to get the tests pass here.

I'm not sure here, do we usually wait for a new release of the CLI so the tests here can pass with that? Or would it be preferable to just make the action install the particular SHA?

Frassle commented 9 months ago

I'm not sure here, do we usually wait for a new release of the CLI so the tests here can pass with that? Or would it be preferable to just make the action install the particular SHA?

It's fine to depend on the sha once it's merged to master. We'd need to release a tag for yaml and update pu/pu's dependency as well to actually get this fix shipped.

tgummerer commented 9 months ago

Closing in favor of https://github.com/pulumi/pulumi-yaml/pull/511.