This repository has been archived by the owner on Oct 12, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 9
Properly support enum Names in resources #326
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Porges
requested review from
babbageclunk,
devigned,
matthchr and
theunrepentantgeek
as code owners
November 12, 2020 22:55
Porges
force-pushed
the
the-resource-named-default
branch
from
November 12, 2020 22:56
e625d17
to
e197bab
Compare
Porges
commented
Nov 12, 2020
hack/generator/pkg/astmodel/armconversion/convert_from_arm_function_builder.go
Outdated
Show resolved
Hide resolved
Porges
commented
Nov 13, 2020
Porges
changed the title
Better support for non-string Names in resources
Properly support non-string Names in resources
Nov 16, 2020
Porges
changed the title
Properly support non-string Names in resources
Properly support enum Names in resources
Nov 16, 2020
matthchr
approved these changes
Nov 17, 2020
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.
Left some comments, but looks good
hack/generator/pkg/astmodel/armconversion/convert_to_arm_function_builder.go
Outdated
Show resolved
Hide resolved
hack/generator/pkg/codegen/pipeline_apply_kubernetes_resource_interface.go
Outdated
Show resolved
Hide resolved
iface, err := astmodel.NewKubernetesResourceInterfaceImpl(idFactory, specObj) | ||
if err != nil { | ||
return nil, errors.Wrapf(err, "Couldn't implement Kubernetes resource interface for %q", spec.Name()) | ||
// this is really very ugly; a better way? |
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 could make WithKubernetesResourceInterfaceImpl
return a triplet with one return being an optional updated SpecType
. It makes it a bit more obvious from the function signature that there's a possible modification? Or if the triplet result is too ugly you could make a little struct for it.
It's honestly not lots better but might be a bit better.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Naming changes
Don't convert all resource Spec
Name
properties tostring
, and instead allow the following cases:An enum with one value generates no
AzureName
property in the CRD as it cannot be changed. AnAzureName()
function is provided forConvertToArm
to invoke, which returns the fixed value.This only triggers for one resource currently:
HostingEnvironmentsMultiRolePool
which must have a name ofdefault
An enum with multiple values generates an
AzureName
property with an enum value. AnAzureName()
function is provided forConvertToArm
which casts the enum value to string.Example resources for this are:
VaultsAccessPolicies
which allows names ofadd
,remove
, andreplace
ServiceIdentityProvider
which allows names ofaad
,aadB2C
,facebook
,google
,microsoft
, andtwitter
All Spec
Name
properties are still converted tostring
for the corresponding ARM type, as we need to insert a full ARM ID including owning resource IDs, etc.Serialization changes
Split
ArmTransformer
interface into two partsFromArmConverter
/ToArmConverter
and make it so that Status types only implementFromArmConverter
.Other cases
There are other cases to be solved; some services (like Storage) require a Name ending in
default
(which is actually more correct, as the Name is a full URI including parent resources). At the schema level this is enforced via a regex like^.*/default$
. We can recognize these cases and handle them; but that will be a different PR—we don’t yet support type validation properly.