-
Notifications
You must be signed in to change notification settings - Fork 72
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
feat: support for config on multiple types on keys and values #912
base: main
Are you sure you want to change the base?
Conversation
Having thought about it for a minute, should we parse to string here? |
Can you clarify which line you're referring to? |
Oh, like convert to string instead of casting to |
Also, the build is red because the CHANGELOG is missing an entry. |
Just thinking out loud here: If we pass any, we're passing a strongly typed map and telling the compiler to infer the type is a string-to-string map. When a key is accessed, if the AutomationAPI assumes its a string, then we'll have a type error. If we convert the keys from e.g. Boolean to String, then the Automation API will be correctly typed. I scanned the AutoAPI code to see how it handles keys. It looks like it coerses them to a string (even though they're supposed to be strings, but like it our case, users may lie to the compiler). https://github.com/pulumi/pulumi/blob/master/sdk/nodejs/automation/server.ts#L79 So it looks like the |
Ah, looks like the |
Another option would be to revert #813 , then complete pulumi/pulumi#12641 before re-instating #813. I'm tempted by this direction, since I've already made a few bad assumptions while tracing the automation API code 😅 |
What is outlined in this pull request would result in what we had before #813. Since the old codebase only passed along the object to setAllConfig, so would this. Another option could be not to validate the thing. I'll create a quick PR on that, and we can decide 😅 |
So, here's another alternative. #913 removes validation, getting us back to how we did it before #813. |
Thanks very much for the clarification! I like the idea of returning to how things were before #813 just for now to get the release stable. Then, we can fix pulumi/pulumi#12641 and add the changes back. I'll review #913 presently. |
Before #813 we naively accepted all kinds of objects into
config-map
, which has led to users having other types than what we have defined in our restrictive runtypes.I think what is described in #911 should be supported, and as @mikocot describes in a comment there are probably a lot of users that depend on that. Since we're currently reflecting the types that are supported by
stack.setAllConfig
from the Automation API, it need to be extended to support more types. IMHO, it should be fine to merge this pull request as a temporary fix (with theas any
hotfix along with a TODO and issue)fixes #911
blocked by pulumi/pulumi#12641