search

equinix / terraform-provider-equinix

Terraform Equinix provider
https://deploy.equinix.com/labs/terraform-provider-equinix/
MIT License
47 stars 45 forks source link

fix: wrong fabric schemas #688

Open ocobles opened 1 month ago

ocobles commented 1 week ago

Added some comments. You had asked us to help out but I see you're diving into it pretty deeply.

Question, how is Pulumi failing here? Is it that the Terraform spec isn't matching our SwaggerHub spec or is it that the Fabric APIs are returning data that isn't in the SwaggerHub spec? Wanted to sync up on this one.

@thogarty it was related to this issue https://github.com/pulumi/pulumi-terraform-bridge/issues/1951 Pulumi tries to recreate some unmodified resources, I found that they were TypeSet fields with all subfields in the nested schema optional and computed, on the one hand, this was a bug in the pulumi-terraform-bridge plugin which seems to be solved. However, I could also observe that in those resources we were defining all fields as computed and optional some are never computed or they were required.

So taking the example below, AFAIK account_number must be required, actually is a single field in the account schema that is required. However, it is not specified as required in the SwaggerHub specs https://app.swaggerhub.com/apis/equinix-api/fabric/4.13#/SimplifiedAccount. Ideally, this should be well defined in the specs, looking for semi-automated processes to generate the Terraform provider as well as the SDKs. The first problem I see in the specs is that in some of them, the same objects are used for both the POST request and for the GET request/response, so there we cannot even distinguish when a field is read-only, which is what most of them are for the GET.

image

thogarty commented 3 days ago

Added some comments. You had asked us to help out but I see you're diving into it pretty deeply. Question, how is Pulumi failing here? Is it that the Terraform spec isn't matching our SwaggerHub spec or is it that the Fabric APIs are returning data that isn't in the SwaggerHub spec? Wanted to sync up on this one.

@thogarty it was related to this issue pulumi/pulumi-terraform-bridge#1951 Pulumi tries to recreate some unmodified resources, I found that they were TypeSet fields with all subfields in the nested schema optional and computed, on the one hand, this was a bug in the pulumi-terraform-bridge plugin which seems to be solved. However, I could also observe that in those resources we were defining all fields as computed and optional some are never computed or they were required.

So taking the example below, AFAIK account_number must be required, actually is a single field in the account schema that is required. However, it is not specified as required in the SwaggerHub specs https://app.swaggerhub.com/apis/equinix-api/fabric/4.13#/SimplifiedAccount. Ideally, this should be well defined in the specs, looking for semi-automated processes to generate the Terraform provider as well as the SDKs. The first problem I see in the specs is that in some of them, the same objects are used for both the POST request and for the GET request/response, so there we cannot even distinguish when a field is read-only, which is what most of them are for the GET.

image

I'm with you that the Fabric Specs are not ideal. We're trying to sort them out as we go along and the collaboration with the many Fabric teams doesn't always get the correct result. The biggest issue being exactly what you've said where we have the same API models that are used as Request/Response which causes a lot of unintended side effects for client interfaces and developer documentation. It doesn't impact the development teams for Back End/Front End quite as much because of how tight their feedback loop with each other and Product on these issues.

I'll cite this issue as a reason for instigating change in the schema from those teams and hopefully get that separated so that the model attribute behaviors can be rectified.

Coming back to your example though, why do you see AccountNumber in the CloudRouter schema as being required? Couldn't follow your message the first time around so looking for more clarity.