-
Notifications
You must be signed in to change notification settings - Fork 156
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
Correctly set the alt type for aws_cloudwatch_log_resource_policy
#3743
Correctly set the alt type for aws_cloudwatch_log_resource_policy
#3743
Conversation
- `aws_cloudwatch_log_resource_policy.policy_document` - `aws_ecr_registry_policy.policy` - `aws_sns_topic_policy.policy` This is a breaking change for Go and Typescript if users were reading the `PolicyDocument` field on LogResourcePolicyArgs or on LogResourcePolicy. This was discovered as part of pulumi/pulumi-terraform-bridge#1758.
37abf59
to
86621a5
Compare
@@ -150,13 +150,13 @@ func (RegistryPolicyState) ElementType() reflect.Type { | |||
|
|||
type registryPolicyArgs struct { | |||
// The policy document. This is a JSON formatted string. | |||
Policy string `pulumi:"policy"` | |||
Policy interface{} `pulumi:"policy"` |
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.
I don't think this is exactly breaking as it interface{}
slot would still accept string, right? So earlier programs likely still continue working.
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.
If you write to RegistryPolicyArgs.Policy
, it is not breaking. If you read from RegistryPolicyArgs.Policy
, then it is breaking. I believe that writes are the common case, but you can read from args.
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.
Yeah .. I think so. We haven't discussed how strict we want to be here, but I think we shouldn't count these changes as breaking IMO.
I'd be fine with merging because I think it won't break 99.9% of programs, curious on what @corymhall thinks. There is something really improving here for TypeScript users but not so much for Go, Go decides to emit |
Filed p/p enhancement around Go support. |
Does the PR have any schema changes?Does the PR have any schema changes?Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
Is README.md missing any configuration options?
Please add a description for each of these options to |
This is a breaking change for Go and Typescript if users were reading the
PolicyDocument
field on LogResourcePolicyArgs or on LogResourcePolicy. If we don't want to take the change, we can remove the override entirely (semantically equivalent to what we have now in master).This was discovered as part of pulumi/pulumi-terraform-bridge#1758.