-
Notifications
You must be signed in to change notification settings - Fork 47
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
fix: wrong fabric schemas #688
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: ocobleseqx <[email protected]>
Signed-off-by: ocobleseqx <[email protected]>
…lgorithm Signed-off-by: ocobleseqx <[email protected]>
Signed-off-by: ocobleseqx <[email protected]>
Signed-off-by: ocobleseqx <[email protected]>
Signed-off-by: ocobleseqx <[email protected]>
Signed-off-by: ocobleseqx <[email protected]>
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.
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.
- `type` (String) Supported Network types - EVPLAN, EPLAN, IPWAN | ||
- `project` (Block Set, Min: 1, Max: 1) Fabric Network project (see [below for nested schema](#nestedblock--project)) | ||
- `scope` (String) Fabric Network scope. One of [REGIONAL GLOBAL LOCAL] | ||
- `type` (String) Supported Network types. One of [REGIONAL GLOBAL LOCAL] |
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 have the correct enum in the validation and description string of the resource for the type. It just needs to have a doc regeneration to pick it up here so that scope and type are not the same.
@@ -16,3 +18,24 @@ var ( | |||
StringIsSpeedBand = validation.StringMatch(regexp.MustCompile("^[0-9]+(MB|GB)$"), "SpeedBand should consist of digit followed by MB or GB") | |||
StringIsCountryCode = stringvalidator.RegexMatches(regexp.MustCompile("(?i)^[a-z]{2}$"), "Address country must be a two letter code (ISO 3166-1 alpha-2)") | |||
) | |||
|
|||
// StringInEnumSlice checks if a string is in a slice of enum | |||
func StringInEnumSlice[T ~string](valid []T, ignoreCase bool) func(i interface{}, k string) (warnings []string, errors []error) { |
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 is an incredibly valuable addition. Might be worth opening a separate PR just for this so that we can use it in additional feature development.
@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 So taking the example below, AFAIK |
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. |
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.
Lot of good changes in this PR. We've moved a couple of resources into the internal package and plan on doing the same for others soon. Might be better to put some of this updates into single resource PRs so that it's easier to review/approve/get merged with other in flight work.
Description: "Priority type - Primary or Secondary", | ||
}, | ||
} | ||
} |
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.
Nit, no closing newline at the end of the file.
No description provided.