-
Notifications
You must be signed in to change notification settings - Fork 79
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
Adopt plain types in the schema to match the implementation #819
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
region?: pulumi.Input<aws.Region>; | ||
profile?: pulumi.Input<string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are non-plain Input<T>
in the schema:
pulumi-eks/provider/cmd/pulumi-gen-eks/main.go
Lines 619 to 624 in c6f1fce
"region": { | |
TypeSpec: schema.TypeSpec{Type: "string"}, // TODO: enum: consider typing as `aws.Region` | |
}, | |
"profile": { | |
TypeSpec: schema.TypeSpec{Type: "string"}, | |
}, |
And they can easily be made Input<T>
in the implementation, as they're just passed along through getRoleProvider
, which passes them as inputs when creating aws.Provider
, which already accepts Input<T>
for these:
pulumi-eks/nodejs/eks/cluster.ts
Lines 331 to 333 in c6f1fce
const creatorProvider = new aws.Provider(`${name}-eksClusterCreatorEntity`, { | |
region: region, | |
profile: profile, |
94a1885
to
d16c5c8
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
There are cases where inputs to some components in the TS implementation are plain, but the schema doesn’t indicate they are plain (EKS was originally schematized before plain values were supported in the schema), so it’s possible someone could pass an `Output<T>` for these from multi-lang, which may result in problems in the implementation if it isn't expecting an `Output<T>` value.
d16c5c8
to
76ad0b4
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great 👏
Might be a couple more of these that we can make inputty, but that's a non-breaking change we can revisit another day.
There are cases where inputs to some components in the TS implementation are plain, but the schema doesn’t indicate they are plain (EKS was originally schematized before plain values were supported in the schema), so it’s possible someone could pass an
Output<T>
for these from multi-lang, which may result in problems in the implementation if it isn't expecting anOutput<T>
value.Fixes #817