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

[sdk/go] Simplify Apply method options to reduce binary size #6607

Merged
merged 6 commits into from Mar 24, 2021

Conversation

lukehoban
Copy link
Member

@lukehoban lukehoban commented Mar 24, 2021

[Note: This PR is intended to be delivered as part of 3.0, and will be moved to target the feature-3.0 branch once that branch is up to date with master.]

Make three breaking changes to the Pulumi Go SDK to (a) dramatically reduce the size and build-time for Pulumi Go programs and (b) simplify the options for Apply in Go to offer a more consistent programming model.

  • Remove o.Apply<TypeName>(...) methods => use o.ApplyT(...) instead
  • Remove o.Apply(...) methods => use o.ApplyT(...) instead
  • Move o.IsSecret() method => use pulumi.IsSecret(o) instead

For a common Go program (the Pulumi Kubernetes Go template for example), this reduces binary size by 77% (from 131.2MB to 30.7MB).

These changes also reduce the options for Apply in Pulumi's Go SDK from 3 options to just 1. Users will always use ApplyT, and can express the type of the output they get from the ApplyT call as a type assertion. This was the most commonly used style in existing codebases, and is not the single standardized option. Both Apply and Apply<TypeName> were previously thin sugar over ApplyT - but that sugar led to confusion in the programming model, and significant unintended size increases.

Fixes #6592.
Resolves #6356 (no longer necessary).
Resolves #6591 (addresses most sources of large binary size).

@lukehoban lukehoban added the impact/breaking Fixing this issue will require a breaking change label Mar 24, 2021
Copy link
Contributor

@leezen leezen left a comment

Choose a reason for hiding this comment

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

Changes LGTM. I'm going to avoid hitting 'approve' for now to make it harder to accidentally merge into master.

@lukehoban lukehoban changed the base branch from master to feature-3.0 March 24, 2021 09:17
@stack72 stack72 force-pushed the feature-3.0 branch 2 times, most recently from bc1e73a to 9668cd2 Compare March 24, 2021 11:26
@stack72 stack72 force-pushed the lukehoban/removeapplytypename branch from 4eb61e0 to 85c05e4 Compare March 24, 2021 11:27
@stack72
Copy link
Contributor

stack72 commented Mar 24, 2021

@lukehoban I've rebased this for you based on the new feature-3.0 branch - this is a simpler merge now :D

@t0yv0
Copy link
Member

t0yv0 commented Mar 24, 2021

LGTM. Flaky tests in CI? Something shows as red.

@stack72
Copy link
Contributor

stack72 commented Mar 24, 2021

@t0yv0 yeah this is because of the new minVersionChecking that is breaking our CI tests

@stack72 stack72 force-pushed the feature-3.0 branch 3 times, most recently from 6050cbe to 72bf1b0 Compare March 24, 2021 20:01
komalali and others added 6 commits March 24, 2021 20:04
Reduces size for simple Kuberntes Go app from 131MB to 37.5MB - a 71% size reduction.

Code that used `o.ApplyString(func(...)...)` must use  `o.ApplyT(func(...)...).(StringOutput)` instead.
@stack72 stack72 force-pushed the lukehoban/removeapplytypename branch from 85c05e4 to 7035c97 Compare March 24, 2021 20:05
@stack72 stack72 merged commit e2f9a3b into feature-3.0 Mar 24, 2021
@stack72 stack72 deleted the lukehoban/removeapplytypename branch March 24, 2021 20:46
stack72 pushed a commit that referenced this pull request Mar 31, 2021
viveklak pushed a commit that referenced this pull request Apr 3, 2021
stack72 pushed a commit that referenced this pull request Apr 5, 2021
stack72 pushed a commit that referenced this pull request Apr 5, 2021
stack72 pushed a commit that referenced this pull request Apr 13, 2021
stack72 pushed a commit that referenced this pull request Apr 14, 2021
stack72 pushed a commit that referenced this pull request Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/breaking Fixing this issue will require a breaking change
Projects
None yet
6 participants