-
Notifications
You must be signed in to change notification settings - Fork 2
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
Upgrade terraform-provider-equinix to v1.34.0 #89
Conversation
Does the PR have any schema changes?Does the PR have any schema changes?Found 4 breaking changes: Resources
Types
Maintainer note: consult the runbook for dealing with any breaking changes. |
@@ -31,7 +29,7 @@ public final class GetProjectBgpConfig { | |||
* @return Password for BGP session in plaintext (not a checksum). | |||
* | |||
*/ | |||
private @Nullable String md5; | |||
private String md5; |
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 understand what this change corresponds to on the terraform side. The md5 attribute is still optional in the terraform provider, so I would expect it to remain nullable in Pulumi.
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.
That's the output, and we are setting always a value for BGPConfig in https://github.com/equinix/terraform-provider-equinix/blob/d63009a723d5f9101cf7ee7ede863eef9a4fb324/internal/resources/metal/project/models.go#L97
even if it is an empty list so it makes sense for me.
In the project resource definition is still Nullable
private Output</* @Nullable */ ProjectBgpConfig> bgpConfig; |
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.
When using the terraform provider directly, at least, MD5 should still be nil on output if the user provides a BGP config but does not specify an MD5; that behavior was restored in 1.33.1 and should continue to exist in 1.34.0.
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'm on board to write this off as a Pulumi thing, but my concern is that this may indicate there's still something wrong with how we're handling null MD5 in the Terraform provider.
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.
you right I was checking bgpConfig instead of the specific md5 field
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.
It sounds like this is a reflection of the issue we ran into during the TF framework provider conversion, where we couldn't define a nested schema for BGP config in the data source, so it doesn't have the same behavior as the converted resource. If that's the case, this is expected and there's not really anything we can do about it.
This PR was generated via
$ upgrade-provider equinix/pulumi-equinix --upstream-provider-name terraform-provider-equinix
.