Change policyArns in RoleWithPolicyArgs to accept eventual types #1276
Conversation
Previously the policyArns attribute in RoleWithPolicyArgs only accepted plain types as inputs. This forced users to use workarounds like using `apply` in order to configure policies created in the same pulumi program. Fixes #1197
|
|
||
| /// <summary> | ||
| /// ARNs of the policies to attach to the created role. | ||
| /// </summary> | ||
| public List<string> PolicyArns | ||
| public InputList<string> PolicyArns |
There was a problem hiding this comment.
public sealed class InputList<T> : Input<ImmutableArray<T>>, IEnumerable, IAsyncEnumerable<Input<T>>
This is breaking I think.
There was a problem hiding this comment.
I think dotnet has an implict conversion from List<T> to InputList<T>
There was a problem hiding this comment.
I will try it out just to be sure. I was also under the impression, but better safe than sorry.
There was a problem hiding this comment.
Oh that's true, I forgot about implicit conversions. That may be nice.
| @@ -2129,7 +2129,7 @@ type RoleWithPolicyArgs struct { | |||
| // ARN of the policy that is used to set the permissions boundary for the role. | |||
| PermissionsBoundary pulumi.StringPtrInput `pulumi:"permissionsBoundary"` | |||
| // ARNs of the policies to attach to the created role. | |||
| PolicyArns []string `pulumi:"policyArns"` | |||
| PolicyArns pulumi.StringArrayInput `pulumi:"policyArns"` | |||
| * @return builder | ||
| * | ||
| */ | ||
| public Builder policyArns(List<String> policyArns) { |
There was a problem hiding this comment.
This is not breaking because a new overload is generated with the same sig.
| @@ -310,7 +310,7 @@ export namespace awsx { | |||
| /** | |||
| * ARNs of the policies to attach to the created role. | |||
| */ | |||
| policyArns?: string[]; | |||
| policyArns?: pulumi.Input<pulumi.Input<string>[]>; | |||
There was a problem hiding this comment.
I suspect TypeScript has enough magic this is not breaking.
There was a problem hiding this comment.
Yeah in typescript it is fine because Input is a type alias for T | Promise<T> | OutputInstance<T>
|
@t0yv0 You're absolutely correct about the breaking changes. I had started a thread about this on the original issue after I noticed it (sorry for not updating the PR yet!): #1197 (comment) I'm thinking about adding a new parameter and deprecating the old one, do you have any other ideas to add this quality of life improvement without causing pain for other users? |
| @@ -69,7 +69,7 @@ export function defaultRoleWithPolicies( | |||
| ): { | |||
| roleArn?: pulumi.Output<string>; | |||
| role?: aws.iam.Role; | |||
| policies?: aws.iam.RolePolicyAttachment[]; | |||
| policies?: pulumi.Output<aws.iam.RolePolicyAttachment[]>; | |||
There was a problem hiding this comment.
I thought there was some magic way to do this change for the SDKs while still receiving un-wrapped values in the provider but I may be mistaken. If this works it's good.
I slightly worry about provisioning resources under apply inside the component resource, but I might need some platform team help to find references to how this works.
https://www.pulumi.com/docs/concepts/resources/components/#registering-component-outputs
Normally there is a call registerOutputs at the end of the constructor.
We don't seem to be calling it for ec2TaskDefinition:
https://github.com/pulumi/pulumi-awsx/blob/master/awsx/ecs/ec2TaskDefinition.ts#L32
If we're doing apply then children may register out-of-order that is after the component resource is done constructing and I vaguely recall this may cause bugs.
|
Since examples/tests aren't changing looks like this may not be under test yet, would it be possible to add a quick end-to-end example using this? |
|
Re: slightly breaking changes like this, I need a reminder CC @mjeffryes on what our breaking change policy is here. It sounds like change is not breaking for TypeScript which may account for most of the user base. I agree with you that if we're strict on breaking changes then a new added property is a good alternative. |
Yes for sure! |
|
We're planning this for the next major version upgrade. Closing the PR for now, we can pick it up again once we're working on the next major version |
Previously the
policyArnsattribute in RoleWithPolicyArgs only accepted plain types as inputs. This forced users to use workarounds like usingapplyin order to configure policies created in the same pulumi program.This PR changes the
policyArnsattribute from a plain string array to an nested Input array (Input<Input<string>[]>) to allow users to pass the whole list as well as individual elements as eventual types.Fixes #1197