Add support for 20240401 AML resources #4237
Conversation
super-harsh
requested review from
davefellows,
theunrepentantgeek,
matthchr and
babbageclunk
as code owners
| @@ -2442,6 +2442,7 @@ objectModelConfiguration: | |||
| UserAssignedIdentity: | |||
| $armReference: false # Actually, this *IS* a resource id, but we want to avoid breaking changes so we're fibbing here | |||
| KeyVaultProperties: | |||
| $nameInNextVersion: EncryptionKeyVaultProperties | |||
| KeyVaultArmId: | |||
| $armReference: false # Actually, this *IS* a resource id, but we want to avoid breaking changes so we're fibbing here | |||
There was a problem hiding this comment.
Since we're already taking breaking changes here for PrivateKeyData and PublicKeyData, should we fix this up too? @matthchr, what do you think?
There was a problem hiding this comment.
I think we should avoid gratuitous breaking change unless there's a security reason behind it (like for the other properties).
For this one, ideally we fix this in a new API version - if we can do that here then yes that would be good to do.
v2/internal/controllers/crd_machinelearningservices_20240401_test.go
Outdated
Show resolved
Hide resolved
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| package controllers_test |
There was a problem hiding this comment.
I think we've been naming these files following the new naming pattern which is _test.go - so in this case machinelearning_workspaces_20240401_test.go.
I don't think changing this file name impacts the test recordings at all so it should be easy to change.
v2/samples/machinelearningservices/v1api20240401/v1api20240401_workspacesconnection.yaml
Outdated
Show resolved
Hide resolved
| │ │ │ │ │ ├── "DenseProd" | ||
| │ │ │ │ │ ├── "DevTest" | ||
| │ │ │ │ │ └── "FastProd" | ||
| │ │ │ │ ├── LoadBalancerSubnetReference: *genruntime.ResourceReference |
There was a problem hiding this comment.
I think we need a conversion for this, between the old value (which was a string) and the new value (which is a Reference). D owe have one?
There was a problem hiding this comment.
Good point! I didn't add one, doesn't generator handle this case?
If not, since we're taking the breaking change anyway on this resource, should we mark this property as a ResourceReference in the previous version as well or write a custom conversion? WDYT?
There was a problem hiding this comment.
My 2c is we should do custom conversion for anything that we can, to reduce the breakages between versions for folks
There was a problem hiding this comment.
Agreed. In the past we've handled this by leaving the old version with *string, using *genruntime.ResourceReference in the new version, and hand-crafting the conversion between the two.
This is done by implementing one of the augment* interfaces to customize the conversion.
You can search for existing examples with
$ cd v2/api
$ git grep -in augment -- '*.go' ':(exclude)*_gen.go'
What this PR does / why we need it:
This PR includes latest version of AML resources. Consider this as a starting point to support further AML resources in next iteration.
Special notes for your reviewer:
This PR includes breaking changes for
WorkspacesCompute20210701 API version below:If applicable: