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] Fix ElementType for nested collection types. #8535

Merged
merged 1 commit into from Dec 3, 2021
Merged

[codegen/go] Fix ElementType for nested collection types. #8535

merged 1 commit into from Dec 3, 2021

Conversation

pgavlin
Copy link
Member

@pgavlin pgavlin commented Dec 2, 2021

The element types for nested collection types are currently incorrect.

For arrays, we generate an element type of []FooArgs, where
FooArgs is an args type for FooInput. Instead, the element type
should be []Foo, where Foo is the element type of FooInput.

For maps, we generate an element type of FooArgs. Instead, the element
type should be map[string]Foo, where Foo is the element type of
FooInput.

These changes correct the element types. This is prep for #7943.

@pgavlin pgavlin requested review from t0yv0 and iwahbe December 2, 2021 22:34
@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-random with merge commit e25aefe

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azuread with merge commit e25aefe

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-kubernetes with merge commit e25aefe

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-gcp with merge commit e25aefe

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-random with merge commit 365940a

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azuread with merge commit 365940a

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-kubernetes with merge commit 365940a

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azure with merge commit e25aefe

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-gcp with merge commit 365940a

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-aws with merge commit e25aefe

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azure with merge commit 365940a

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-aws with merge commit 365940a

@codecov
Copy link

codecov bot commented Dec 2, 2021 •
edited

Codecov Report

Merging #8535 (bfb2cab) into master (05e26e3) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head bfb2cab differs from pull request most recent head 96ca85c. Consider uploading reports for the commit 96ca85c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8535      +/-   ##
==========================================
+ Coverage   58.63%   58.64%   +0.01%     
==========================================
  Files         634      634              
  Lines       96865    96872       +7     
  Branches     1378     1378              
==========================================
+ Hits        56794    56808      +14     
+ Misses      36808    36798      -10     
- Partials     3263     3266       +3
Impacted Files Coverage Δ
pkg/codegen/go/gen.go 88.52% <100.00%> (+0.02%) ⬆️
sdk/go/common/resource/properties.go 82.59% <0.00%> (-1.11%) ⬇️
pkg/codegen/hcl2/model/type_object.go 89.18% <0.00%> (-0.55%) ⬇️
sdk/nodejs/automation/localWorkspace.ts 73.64% <0.00%> (-0.39%) ⬇️
sdk/go/common/util/ciutil/github_actions.go 73.52% <0.00%> (+38.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abfae57...96ca85c. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azure-native with merge commit e25aefe

iwahbe
iwahbe reviewed Dec 2, 2021
View reviewed changes
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

Can you explain to my why this isn't a breaking change? Otherwise this looks good.

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azure-native with merge commit 365940a

@pgavlin
Copy link
Member Author

pgavlin commented Dec 2, 2021

Can you explain to my why this isn't a breaking change? Otherwise this looks good.

Strictly speaking, this is a breaking change. In particular, it will break code that does an ApplyT with a callback that returns a Foo{Array,Map} and type asserts to Foo{Array,Map}MapOutput or Foo{Array,Map}ArrayOutput. I strongly doubt that such code exists in the wild, however: attempting to do anything with the result is unlikely to work, as the type of the resolved value of the output will not match the shape expected by its consumers if it is passed to another Apply or to a resource as an input. The way in which this is a breaking change is that it makes these output/input types usable by correcting the element type s.t. the resolved value of an affected output matches the shape expected by its consumers.

@pgavlin
Copy link
Member Author

pgavlin commented Dec 2, 2021

(and because this is a break that is making previously unusable types usable, I think we should take it)

@pgavlin pgavlin requested a review from stack72 December 2, 2021 23:40
@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-random with merge commit 86bd894

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azuread with merge commit 86bd894

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-kubernetes with merge commit 86bd894

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-gcp with merge commit 86bd894

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Diff for pulumi-azure with merge commit 86bd894

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

Diff for pulumi-aws with merge commit 86bd894

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

Diff for pulumi-azure-native with merge commit 86bd894

@iwahbe iwahbe self-requested a review December 3, 2021 01:07
@emiliza emiliza added capacity/quality kind/task Work that's part of an ongoing epic size/S Estimated effort to complete (1-2 days). labels Dec 3, 2021
@emiliza emiliza added this to the 0.65 milestone Dec 3, 2021
@pgavlin
[codegen/go] Fix ElementType for nested collection types.
96ca85c 
The element types for nested collection types are currently incorrect.

For arrays, we generate an element type of `[]FooArgs`, where
`FooArgs` is an args type for `FooInput`. Instead, the element type
should be `[]Foo`, where `Foo` is the element type of `FooInput`.

For maps, we generate an element type of `FooArgs`. Instead, the element
type should be `map[string]Foo`, where `Foo` is the element type of
`FooInput`.

These changes correct the element types.
@github-actions
Copy link

github-actions bot commented Dec 3, 2021

Diff for pulumi-azuread with merge commit 59c7d49

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

Diff for pulumi-gcp with merge commit 59c7d49

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

Diff for pulumi-random with merge commit 59c7d49

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

Diff for pulumi-kubernetes with merge commit 59c7d49

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

Diff for pulumi-azure with merge commit 59c7d49

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

Diff for pulumi-aws with merge commit 59c7d49

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

Diff for pulumi-azure-native with merge commit 59c7d49

@lukehoban
Copy link
Member

Is there an issue tracking the bug this fixes? I'm curious to see concretely the code that doesn't work

@pgavlin
Copy link
Member Author

pgavlin commented Dec 3, 2021

Is there an issue tracking the bug this fixes? I'm curious to see concretely the code that doesn't work

There is not. This is something I came across while working on #7943.

@pgavlin
Copy link
Member Author

pgavlin commented Dec 3, 2021

@lukehoban this is an example of some code that does not work: https://gist.github.com/pgavlin/bfcc0ba03e8a2b2e7dece4cdbb9c1b1c

The ToSomeOtherObjectArrayArrayOutput panics because the element type of SomeOtherObjectArrayArrayOutput does not match the resolved type of SomeOtherObjectArrayArray:

gh8535 ❯ ./gh8535 
panic: interface conversion: pulumi.Output is pulumi.AnyOutput, not main.SomeOtherObjectArrayArrayOutput

goroutine 1 [running]:
main.SomeOtherObjectArrayArray.ToSomeOtherObjectArrayArrayOutputWithContext(...)
	/Users/pgavlin/dev/pulumi/repros/gh8535/main.go:199
main.SomeOtherObjectArrayArray.ToSomeOtherObjectArrayArrayOutput(...)
	/Users/pgavlin/dev/pulumi/repros/gh8535/main.go:195
main.main()
	/Users/pgavlin/dev/pulumi/repros/gh8535/main.go:299 +0x145

@pgavlin
Copy link
Member Author

pgavlin commented Dec 3, 2021

In contrast, the version with correct element types does not panic: https://gist.github.com/pgavlin/2511d0e4cadf1232cfd44682bd5589ae

@pgavlin pgavlin merged commit e824ba4 into master Dec 3, 2021
@pulumi-bot pulumi-bot deleted the pgavlin/nestedElementTypes branch December 3, 2021 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stack72 stack72 stack72 approved these changes

@iwahbe iwahbe iwahbe approved these changes

@t0yv0 t0yv0 Awaiting requested review from t0yv0

Assignees
No one assigned
Labels
kind/task Work that's part of an ongoing epic size/S Estimated effort to complete (1-2 days).
Projects
None yet
Milestone
0.65
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pgavlin @lukehoban @stack72 @iwahbe @emiliza