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

respect types for config set in the yaml file #509

Closed
wants to merge 5 commits into from

Conversation

tgummerer
Copy link
Contributor

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

@tgummerer tgummerer requested a review from a team October 10, 2023 13:57
@Frassle
Copy link
Member

Frassle commented Oct 10, 2023

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 tgummerer force-pushed the tg/config-node-reading branch 2 times, most recently from c7d9d9a to 917afbd Compare October 13, 2023 15:34
If they are available we should use the config types we get through
the RPC call.  Do that.
@tgummerer tgummerer force-pushed the tg/config-node-reading branch 2 times, most recently from d52dd18 to 5d03123 Compare October 16, 2023 08:29
@tgummerer
Copy link
Contributor Author

I think this is ready for another review, now taking the types from the RPC call. Before merging this it needs pulumi/pulumi#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
Copy link
Member

Frassle commented Oct 16, 2023

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
Copy link
Contributor Author

Closing in favor of #511.

@tgummerer tgummerer closed this Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quoted variable values incorrectly converted to integers
2 participants