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

[codegen/go] allow plain default types #8254

Merged
merged 15 commits into from
Oct 27, 2021
Merged

[codegen/go] allow plain default types #8254

merged 15 commits into from
Oct 27, 2021

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Oct 19, 2021 •
edited
Loading

Description

Help go combine the plain and default schema fields.

Fixes #7196

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

@iwahbe iwahbe self-assigned this Oct 19, 2021
@github-actions
Copy link

Diff for pulumi-random with merge commit 7fa43bd

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 7fa43bd

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 7fa43bd

@iwahbe iwahbe requested a review from justinvp October 19, 2021 08:29
@iwahbe iwahbe changed the title Iwahbe/7196/allow plain default types [codegen/go] allow plain default types Oct 19, 2021
@github-actions
Copy link

Diff for pulumi-gcp with merge commit 7fa43bd

@iwahbe iwahbe requested a review from a team October 19, 2021 08:31
@github-actions
Copy link

Diff for pulumi-random with merge commit 7549f03

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 7549f03

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 7549f03

@github-actions
Copy link

Diff for pulumi-random with merge commit f54517a

@github-actions
Copy link

Diff for pulumi-azuread with merge commit f54517a

@github-actions
Copy link

Diff for pulumi-aws with merge commit 7fa43bd

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 7549f03

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit f54517a

@github-actions
Copy link

Diff for pulumi-gcp with merge commit f54517a

@github-actions
Copy link

Diff for pulumi-azure with merge commit 7fa43bd

@github-actions
Copy link

Diff for pulumi-aws with merge commit 7549f03

@github-actions
Copy link

Diff for pulumi-aws with merge commit f54517a

@github-actions
Copy link

Diff for pulumi-azure with merge commit 7549f03

@github-actions
Copy link

Diff for pulumi-azure with merge commit f54517a

@github-actions
Copy link

Diff for pulumi-gcp with merge commit efb7424

@github-actions
Copy link

Diff for pulumi-aws with merge commit efb7424

@github-actions
Copy link

Diff for pulumi-azure with merge commit efb7424

@iwahbe iwahbe requested a review from a team October 20, 2021 23:28
@github-actions
Copy link

Diff for pulumi-azure-native with merge commit efb7424

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 500d74e

@github-actions
Copy link

Diff for pulumi-random with merge commit 500d74e

@iwahbe
Copy link
Member Author

iwahbe commented Oct 25, 2021

@t0yv0 I personally prefer the original approach, mostly because of the type safety that you mentioned. Generating the types manually (as opposed to inferring them) might be more error prone, but it will incur compile time errors instead of runtime errors. That means we need only compile each generated SDK to be sure it will work. I think this is overall safer (less likely to ship users invalid code). I would be curious to what others think though.

BTW: I appreciate you taking the time to come up with what you did. It is definitely an idea worth considering.

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 500d74e

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 500d74e

@github-actions
Copy link

Diff for pulumi-azure with merge commit 500d74e

@github-actions
Copy link

Diff for pulumi-aws with merge commit 500d74e

@github-actions
Copy link

Diff for pulumi-azure-native with merge commit 500d74e

@iwahbe iwahbe requested a review from t0yv0 October 26, 2021 00:36
@t0yv0
Copy link
Member

t0yv0 commented Oct 26, 2021

@iwahbe generally I err on the type safety side also; the reasons to exploring reflection more:

  • our context is that there are significant issues with SDK bloat. The size of the generated code is a scarce resource in e.g. pulumi-azure-native. There is also a linker tax we impose on all the users (time to run pulumi up spent in Go linker) any time we increase the generated code size.

  • "direct" code is a bit easier/faster to test than codegen code with techniques like https://github.com/flyingmutant/rapid since the tests don't need to loop in Go compiler itself

Reflection is just the only kind of rope Go gives us that can possibly give us the ability to move code out of concrete contexts (specific resources) into the general contexts (SDK) achieving abstraction/code compression.

@t0yv0
Copy link
Member

t0yv0 commented Oct 26, 2021

Please go ahead with your PR, we just may return to the reflection conversation later on when revisiting the Go SDK design principles in some broader context.

justinvp
justinvp approved these changes Oct 26, 2021
View reviewed changes
Copy link
Member

@justinvp justinvp left a comment

Choose a reason for hiding this comment

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

LGTM

@justinvp
Copy link
Member

RE: alternative approach: FWIW, my preference is to keep the original approach for this PR, but I very much appreciate the exploration on the alternative approach. Definitely something to consider and keep in mind as we look for ways to reduce the size of the generated SDKs.

@pgavlin
Copy link
Member

pgavlin commented Oct 26, 2021

Apologies for chiming in late here--between being OOF and being sick, I've fallen pretty far behind in code reviews.

Just to check my understanding: it looks like with these changes we will generate a *T for an optional plain T where T is not already nilable (e.g. *bool for an optional plain boolean). We will then detect the presence or absence of a value based on whether or not the supplied value is nil. Do I have that right? If so, I like that approach 👍

@github-actions
Copy link

Diff for pulumi-random with merge commit 0face74

@github-actions
Copy link

Diff for pulumi-azuread with merge commit 0face74

@github-actions
Copy link

Diff for pulumi-kubernetes with merge commit 0face74

@github-actions
Copy link

Diff for pulumi-gcp with merge commit 0face74

@github-actions
Copy link

Diff for pulumi-aws with merge commit 0face74

@github-actions
Copy link

Diff for pulumi-azure with merge commit 0face74

@github-actions
Copy link

Diff for pulumi-azure-native with merge commit 0face74

@iwahbe iwahbe merged commit 5306415 into master Oct 27, 2021
@pulumi-bot pulumi-bot deleted the iwahbe/7196/allow-plain-default-types branch October 27, 2021 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinvp justinvp justinvp approved these changes

@t0yv0 t0yv0 Awaiting requested review from t0yv0

Assignees

@iwahbe iwahbe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Using Defaults and Plain Inputs in schema
4 participants
@iwahbe @t0yv0 @justinvp @pgavlin