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

feat: support for config on multiple types on keys and values #912

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

simenandre
Copy link
Contributor

@simenandre simenandre commented Apr 11, 2023

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 the as any hotfix along with a TODO and issue)

fixes #911

blocked by pulumi/pulumi#12641

@simenandre
Copy link
Contributor Author

Having thought about it for a minute, should we parse to string here?

@RobbieMcKinstry
Copy link
Contributor

RobbieMcKinstry commented Apr 11, 2023

Having thought about it for a minute, should we parse to string here?

Can you clarify which line you're referring to?

@RobbieMcKinstry
Copy link
Contributor

Oh, like convert to string instead of casting to any?

@RobbieMcKinstry
Copy link
Contributor

Also, the build is red because the CHANGELOG is missing an entry.

@RobbieMcKinstry
Copy link
Contributor

RobbieMcKinstry commented Apr 11, 2023

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 any approach is safe, but that relies on the internals of the AutoAPI to do the right thing. I think coercing the values to a string is safer.

@RobbieMcKinstry
Copy link
Contributor

Ah, looks like the getConfigMap function I referenced points to a different getConfigMap (coming from protobuf)?

@RobbieMcKinstry
Copy link
Contributor

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 😅

@simenandre
Copy link
Contributor Author

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 😅

@simenandre
Copy link
Contributor Author

So, here's another alternative. #913 removes validation, getting us back to how we did it before #813.
Then we can put this PR on hold until pulumi/pulumi#12641 is fixed.

@RobbieMcKinstry
Copy link
Contributor

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.

@simenandre simenandre added blocked The issue cannot be resolved without 3rd party action. awaiting-upstream The issue cannot be resolved without action in another repository (may be owned by Pulumi). labels Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-upstream The issue cannot be resolved without action in another repository (may be owned by Pulumi). blocked The issue cannot be resolved without 3rd party action.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

config-map failing with "Expected string, but was null" in v4.2
2 participants