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.

Sign up for GitHub

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

Jump to bottom

Fix various typegen issues #635

Merged
merged 2 commits into from
Aug 22, 2019
Merged

Fix various typegen issues #635

merged 2 commits into from
Aug 22, 2019

Conversation

hausdorff
Copy link
Contributor

@hausdorff hausdorff commented Jul 13, 2019 •
edited
Loading

Fixes #444.
Fixes #728.

@hausdorff hausdorff requested review from lblackstone and pgavlin July 13, 2019 18:35
@hausdorff hausdorff force-pushed the hausdorff/explicit-ns branch from 34eac1e to be6be6d Compare July 13, 2019 18:37
@hausdorff
Copy link
Contributor Author

Oops, forgot Python. Will come back for it soon...

@hausdorff hausdorff changed the title Allow .metadata.namespace to be specified with core.v1.Namespace Fix various typegen issues Jul 13, 2019
pgavlin
pgavlin reviewed Jul 16, 2019
View reviewed changes
pkg/gen/nodejs-templates/kind.ts.mustache Outdated
// In Kubernetes, `.metadata.namespace` must be a `string`. In Pulumi, we allow users to
// pass in `string | core.v1.Namespace`. Thus, if the user has provided a
// `core.v1.Namespace`, we convert it to a `string` representing that namespace's name.
args &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this code just won't work at all:

  • The result of the apply is never awaited
  • Mutating the inputs is probably not a good idea (what if they are shared amongst multiple resources?) if it works at all (and it may not, cc @lukehoban)
  • Because core.v1.Namespace.metadata.name is itself an output, we may lose dependency information if it is injected inside an apply (not 100% sure here, but this definitely happens when an output is resolved to another output)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should work because the name is guaranteed to be present after the Check, but you're right that this is not acceptable to ship. Let's fix.

Copy link
Contributor Author

@hausdorff hausdorff Jul 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've cleaned this up a bit -- I'm still on the fence about whether we should move unpackNamespace out into a utility function in some other file. I'm less concerned than normal because it's all code-gen'd, but if there was a good place to put it I'd be open to moving it out.

pgavlin
pgavlin previously requested changes Jul 16, 2019
View reviewed changes
Copy link
Member

@pgavlin pgavlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the namespace-related code is broken; I left a comment with details.

@hausdorff hausdorff force-pushed the hausdorff/explicit-ns branch 2 times, most recently from 8644bbf to 236068c Compare July 16, 2019 06:00
@lukehoban
Copy link
Contributor

@hausdorff Are we going to finish this off and land it? There also appear to be a couple unrelated issues being solved here - and the more critical ones may be less contentious so if we need to land those separately we should do so.

@lukehoban
Copy link
Contributor

@hausdorff What's the status of this?

@hausdorff hausdorff force-pushed the hausdorff/explicit-ns branch 7 times, most recently from 3f1b53b to 69c0fb1 Compare August 22, 2019 17:23
@hausdorff hausdorff force-pushed the hausdorff/explicit-ns branch from 69c0fb1 to 850bd27 Compare August 22, 2019 18:48
@hausdorff hausdorff force-pushed the hausdorff/explicit-ns branch from 850bd27 to 59cf911 Compare August 22, 2019 19:55
@hausdorff hausdorff requested a review from lblackstone August 22, 2019 19:55
@hausdorff hausdorff force-pushed the hausdorff/explicit-ns branch 2 times, most recently from 73db48f to c72c3fc Compare August 22, 2019 20:11
@hausdorff
Enable intellisense in vscode
3c6fa53 
vscode (apparently) intellisense requires direct import of classes in
the __init__.py files for each apiversion. We implement this as such.
@hausdorff hausdorff force-pushed the hausdorff/explicit-ns branch 2 times, most recently from 01e6b1a to 3c6fa53 Compare August 22, 2019 23:39
@hausdorff hausdorff dismissed pgavlin’s stale review August 22, 2019 23:43

Removed the namespace parts that were concerning

@hausdorff hausdorff merged commit e8c9372 into master Aug 22, 2019
@pulumi-bot pulumi-bot deleted the hausdorff/explicit-ns branch August 22, 2019 23:43
@lblackstone lblackstone mentioned this pull request Sep 5, 2019
Closed
@hausdorff hausdorff mentioned this pull request Oct 2, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgavlin pgavlin pgavlin left review comments

@justinvp justinvp justinvp left review comments

@lblackstone lblackstone lblackstone approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Kubernetes Python docs missing many things We should not populate .status in the input types
5 participants
@hausdorff @lukehoban @justinvp @pgavlin @lblackstone