Correctly merge provided opts for k8s resources #850
Conversation
| @@ -85,7 +85,6 @@ import { getVersion } from "../../version"; | |||
| {{#Aliases}} | |||
| { parent: opts.parent, type: "{{.}}", name: name }, | |||
| {{/Aliases}} | |||
| ...((opts && opts.aliases) || []), | |||
There was a problem hiding this comment.
It looks like this will cause us to drop any user-specified aliases. I don't think we want that. Did you try changing ((opts && opts.alaises) || []) to ...(opts.aliases || [])?
There was a problem hiding this comment.
{ parent: opts.parent, type: "kubernetes:apps/v1:Deployment", name: name },
{ parent: opts.parent, type: "kubernetes:apps/v1beta1:Deployment", name: name },
{ parent: opts.parent, type: "kubernetes:apps/v1beta2:Deployment", name: name },
{ parent: opts.parent, type: "kubernetes:extensions/v1beta1:Deployment", name: name },
...(opts.aliases || []),
];
still causes a panic.
There was a problem hiding this comment.
Ah, I'll bet know what the issue is. We're mutating an object that is passed in by the caller. We shouldn't do that. In the case of the original bug, I think we're adding aliases for multiple resources to the same opts dictionary.
There was a problem hiding this comment.
Yep, you're right. I've updated the PR to fix. Looks like we were already handling this properly in the Python SDK.
lblackstone
force-pushed
the
lblackstone/alias-fix
branch
from
762cf5c to
9a954e6
Compare
lblackstone
changed the title
|
@pgavlin @CyrusNajmabadi Can you verify that I'm merging the |
| ] | ||
| }) | ||
|
|
||
| super(MutatingWebhookConfiguration.__pulumiType, name, props, _opts); |
There was a problem hiding this comment.
Indentation looks off above.
Also, our lint rules should disallow use of let and require const here.
There was a problem hiding this comment.
Also - it's pretty ugly to emit all this merging and empty lists and spread across lines. I get that this is generated code - but it is code users will see frequently on Go To Definition - are there reasonable ways to clean this all up a bit?
There was a problem hiding this comment.
Specifically I'm talking about things like this which I see all over the generated output now:
let _opts = pulumi.mergeOptions(opts, {
additionalSecretOutputs: [
],
aliases: [
]
})There was a problem hiding this comment.
Indentation looks off above.
Also, our lint rules should disallow use of
letand requireconsthere.
Fixed
Also - it's pretty ugly to emit all this merging and empty lists and spread across lines. I get that this is generated code - but it is code users will see frequently on Go To Definition - are there reasonable ways to clean this all up a bit?
There may be a way to do so with mustache templates, but I couldn't figure out how when I added the auto-alias feature.
lblackstone
force-pushed
the
lblackstone/alias-fix
branch
from
9a954e6 to
f99bf8c
Compare
lblackstone
force-pushed
the
lblackstone/alias-fix
branch
from
f99bf8c to
d218973
Compare
|
@lukehoban Since your comments were on style/formatting, I'm going to merge now in the interest of making the fix available to test. I can follow up for any further comments. |
| additionalSecretOutputs: [ | ||
| ], | ||
| aliases: [ | ||
| ] |
There was a problem hiding this comment.
this is super unfortunate.
Proposed changes
It seems like the construct we were using in case of empty opts was not working properly for the auto-aliases. This change fixes the linked issue, but I don't quite understand what the problem was.We were not merging user-provided
optsproperly in k8s resources. The bug reported in #849 was caused by mutating the providedoptsobject in multiple resources, which resulted in every resource being aliased to every other resource type sharing that object.This also fixes the same issue for
additionalSecretOutputs.Related issues (optional)
Fixes #849