Swagger only #2084
Swagger only #2084
Conversation
| | Types defined in package "batch" | v1alpha1api20210101 | | ||
| |---------------------------------------------------|---------------------| | ||
| | APIVersion | v1alpha1api20210101 | | ||
| | AutoStorageBaseProperties | v1alpha1api20210101 | | ||
| | AutoStorageBaseProperties_Status | v1alpha1api20210101 | | ||
| | BatchAccount | v1alpha1api20210101 | | ||
| | BatchAccountIdentity | v1alpha1api20210101 | | ||
| | BatchAccountIdentityType | v1alpha1api20210101 | | ||
| | BatchAccountIdentity_Status | v1alpha1api20210101 | | ||
| | BatchAccountIdentity_StatusUserAssignedIdentities | v1alpha1api20210101 | | ||
| | BatchAccountProperties | v1alpha1api20210101 | | ||
| | BatchAccountProperties_Status | v1alpha1api20210101 | | ||
| | BatchAccount_Spec | v1alpha1api20210101 | | ||
| | BatchAccount_Status | v1alpha1api20210101 | | ||
| | EncryptionProperties | v1alpha1api20210101 | | ||
| | EncryptionPropertiesKeySource | v1alpha1api20210101 | | ||
| | EncryptionProperties_Status | v1alpha1api20210101 | | ||
| | KeyVaultProperties | v1alpha1api20210101 | | ||
| | KeyVaultProperties_Status | v1alpha1api20210101 | | ||
| | KeyVaultReference | v1alpha1api20210101 | | ||
| | KeyVaultReference_Status | v1alpha1api20210101 | | ||
| | PoolAllocationMode | v1alpha1api20210101 | | ||
| | PublicNetworkAccessType | v1alpha1api20210101 | |
There was a problem hiding this comment.
How come the list of generated types is so much shorter? 33 vs 21 is a large decrease.
| &dst.FuncLit{ | ||
| Type: &dst.FuncType{ | ||
| Params: &dst.FieldList{List: []*dst.Field{ | ||
| { | ||
| Names: []*dst.Ident{dst.NewIdent("result")}, | ||
| Type: dst.NewIdent(vt.ElementType().String()), | ||
| }, | ||
| }}, | ||
| Results: &dst.FieldList{List: []*dst.Field{ | ||
| {Type: dst.NewIdent(def.Name().Name())}, | ||
| }}, | ||
| }, | ||
| Body: astbuilder.StatementBlock(astbuilder.Returns(astbuilder.CallFunc(def.Name().Name(), dst.NewIdent("result")))), | ||
| }) |
There was a problem hiding this comment.
Pretty sure we have helpers in astbuilder to simplify this.
| } | ||
|
|
||
| /* |
There was a problem hiding this comment.
Should this test still be disabled?
| ResourceTypes ResourceTypes | ||
| OtherTypes astmodel.Types | ||
| } |
| // Uppercase first letter in each part: | ||
| for ix := range nameParts { | ||
| nameParts[ix] = strings.ToUpper(nameParts[ix][0:1]) + nameParts[ix][1:] | ||
| } | ||
|
|
||
| // Singularize last part of name: | ||
| nameParts[len(nameParts)-1] = astmodel.Singularize(nameParts[len(nameParts)-1]) | ||
|
|
||
| name := strings.Join(nameParts, "") |
There was a problem hiding this comment.
I think there's already this functionality in IdentifierFactory - maybe reuse?
| //ResourceGuid: The resourceGuid property of the Virtual Network resource. | ||
| ResourceGuid *string `json:"resourceGuid,omitempty"` | ||
| //Reference: Resource ID. | ||
| Reference *genruntime.ResourceReference `armReference:"Id" json:"reference,omitempty"` |
There was a problem hiding this comment.
Not 100% sure this is right. This looks like the ID field (which should be GET only, I think?) is being turned into a field called Reference. I think this just shouldn't be on the spec at all?
There was a problem hiding this comment.
yes this stuff is still messed up, it's the final keystone piece :D
|
|
||
| //Subnets: A list of subnets in a Virtual Network. | ||
| Subnets []Subnet_Status_VirtualNetwork_SubResourceEmbedded `json:"subnets,omitempty"` | ||
| Subnets []genruntime.ResourceReference `json:"subnets,omitempty"` |
There was a problem hiding this comment.
I think we actually just want to remove this entirely. Might be worth syncing up with me on this though. My current thinking is that these shouldn't exist on the k8s type but should exist on the ARM type, and then we'll write an extension that populates them on the ARM type just before sending to Azure, to deal with #1944.
Although, doing this might solve the problem (by forcing the users to manage their vnets + subnets as one). I am actually not sure what ARM does with a payload that looks like this though, as presumably this ends up being just a collection of ARM IDs rather than a full subnet specification. It's possible that they then clear out other fields on the subnet because they were "unset" here?
| // (Must come after type aliases are resolved) | ||
| // pipeline.ImproveResourcePluralization(), |
There was a problem hiding this comment.
Is this prerequisite declared using pre- or post-requisites?
|
|
||
| // Flatten out any nested resources created by allOf, etc. we want to do this before naming types or things | ||
| // get named with names like Resource_Spec_Spec_Spec: | ||
| pipeline.FlattenResources(), | ||
|
|
||
| // Copy additional swagger-derived information from status into spec | ||
| pipeline.AugmentSpecWithStatus().RequiresPrerequisiteStages("allof-anyof-objects", "addStatusFromSwagger"), | ||
| // pipeline.AugmentSpecWithStatus().RequiresPrerequisiteStages("allof-anyof-objects", "addStatusFromSwagger"), |
There was a problem hiding this comment.
Do we need to keep this commented out code? If so, move the pre-requisite into the stage definition.
|
With #2323 merged, we no longer need to keep this open for reference. |
DRAFT for diffing purposes only
Bonus change: when removing validations from Status types, replace validated types with the underlying type. This means we use plain strings instead of unvalidated enums for Status values.
Fixes #1730, fixes #1554, fixes #1460