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

Output dicts are snake_cased, but input dicts are not? #730

Closed
lukehoban opened this issue Aug 22, 2019 · 11 comments
Closed

Output dicts are snake_cased, but input dicts are not? #730

lukehoban opened this issue Aug 22, 2019 · 11 comments
Assignees
Milestone

Comments

@lukehoban
Copy link
Member

I specify input dict keys in camelCase:

console_service = Service(
    "console",
    spec={
        "selector": console_labels,
        "ports": [{
            "port": 3000,
            "targetPort": 3000
        }],
        "type": "LoadBalancer"
    })

But then if I try to reference them as outputs they are snake_case?

Fails:

pulumi.export("target_port", console_service.spec["ports"][0]["targetPort"])
# KeyError: 'targetPort'

Works:

pulumi.export("target_port", console_service.spec["ports"][0]["target_port"])
@lukehoban lukehoban added this to the 0.26 milestone Aug 22, 2019
@lukehoban
Copy link
Member Author

lukehoban commented Aug 22, 2019

Note that TFGen providers appear to be "correct" here (do the opposite of Kubernetes):

This works:

pulumi.export('bucket_name',  bucket.versioning["mfaDelete"])

This fails:

pulumi.export('bucket_name',  bucket.versioning["mfa_delete"])
# KeyError: 'mfa_delete'

@lblackstone
Copy link
Member

@lukehoban If I understand correctly, the desired behavior is that both the Input/Output dicts are not snake_cased?

@lukehoban
Copy link
Member Author

Right. Python properties are snake_cased, but dict keys are not.

@lukehoban
Copy link
Member Author

We could consider fixing this without a breaking change by returning dicts that have members in both pascalCase and snake_case. That has it's own downsides, but provides an option for fixing this and migrating content over without hard-breaking anyone using the original incorrect key names.

@lblackstone
Copy link
Member

I'll add both casings for 1.0.

@lblackstone
Copy link
Member

Found a related issue that was silently causing certain exports to fail.

import pulumi_kubernetes as k8s

pod = k8s.core.v1.Pod(
    "pod",
    spec={
        "containers": [{"name": "nginx", "image": "nginx"}],
    })

pulumi.export("apiversion", pod.apiVersion)

# No output; expected `apiversion: "v1"`

@lblackstone
Copy link
Member

@lukehoban This turned out to be more complicated than I expected. The specific tf-generated example you chose doesn't appear in the output translation table, but generally, the current behavior seems to be the same (returns snake_cased dict keys).

This translation is performed in the pulumi/pulumi lib, so we'd have to update that method if we wanted to include both casings.

Given that it appears to be consistent behavior across the providers, should we leave this alone until after 1.0?

@lukehoban
Copy link
Member Author

Okay - indeed it does appear there is some deeper confusion here in both pulumi/pulumi and pulumi/pulumi-terraform on what the desired behaviour is. Both of those layers appear to be trying to recursively snake_case output names even through dicts - which I understood not to be intentional. But then they also appear to be failing to correctly do that for common cases (like mfaDelete above).

This also calls into question whether the changes in pulumi/pulumi-terraform#479 are broadly "wrong" (we now document the keys of these nested dicts, and it's not clear we document the names that actually work/exist)?

It's possible that the "by design" here is that both input and output dicts get recursively renamed - in which case there is the opposite bug in the Kubernetes provider - and it is nearly unfixable as every piece of existing Kubernetes Python code critically depends on being able to use camelCased names inside input dicts.

It sounds like we do need to do a deeper review here. @pgavlin Could you take a look at this? I'd love to make sure we're in an understood state before 1.0 here.

@lukehoban lukehoban assigned pgavlin and unassigned lblackstone Aug 27, 2019
@joeduffy
Copy link
Member

Both of those layers appear to be trying to recursively snake_case output names even through dicts - which I understood not to be intentional.

I am also very surprised by this. The only reason we originally rewrote names is that the Go providers need a common casing and, since TS/JS was our first language, that ended up being camel casing. But this should only apply to properties that map directly to actual resource property names, not their values. Dictionaries are opaque to that layer and get mapped to actual underlying resource settings where the user's original casing deeply matters -- as with tags, for example.

@lukehoban
Copy link
Member Author

Dictionaries are opaque to that layer and get mapped to actual underlying resource settings where the user's original casing deeply matters -- as with tags, for example.

I think you are using the word “dictionaries” differently here. I am referring to things which get projected to Python dicts, which is all non-top-level resource inputs and outputs. I believe you are referring to things schematized as Dictionaries in TF which will have user-defined keys. My understanding is that we handle the latter case fine. It is the former case that is broken or at least inconsistent. Hoping @pgavlin can shed light on this.

@lukehoban
Copy link
Member Author

This turned out to be a broader issue. I've summarized our understanding of the issue and the explicit near term changes we expect to try to make in pulumi/pulumi#3151. I'll close this one out as a dupe of that.

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

No branches or pull requests

4 participants