Allow override resource identifier to ARN #163
Conversation
There was a problem hiding this comment.
Nice work on this @RedbackThomson. Do we have a GH issue associated with this, btw? If not, feel free to create one and pop it into the current sprint.
| return arnOut + additionalKeyOut | ||
| } | ||
| return primaryKeyOut + additionalKeyOut | ||
| return primaryKeyConditionalOut + primaryKeyOut + additionalKeyOut |
There was a problem hiding this comment.
These code changes are perfectly fine. I do think you might want to break the function into a couple smaller helper functions in a future PR, though. The function is still pretty longish. :)
There was a problem hiding this comment.
Yeah this method has grown and grown over time. Will break it out when I implement aws-controllers-k8s/community#874
| expected, | ||
| code.SetResourceIdentifiers(crd.Config(), crd, "identifier", "r.ko", 1), | ||
| ) | ||
| } |
| renamedName, _ := r.InputFieldRename( | ||
| op.Name, memberName, | ||
| ) | ||
|
|
||
| isPrimaryIdentifier := memberName == primaryIdentifier | ||
| cleanMemberNames := names.New(renamedName) | ||
| cleanMemberName := cleanMemberNames.Camel | ||
|
|
||
| memberPath := "" | ||
| _, inSpec := r.SpecFields[renamedName] | ||
| _, inStatus := r.StatusFields[renamedName] | ||
| var targetField *model.Field | ||
|
|
||
| specField, inSpec := r.SpecFields[renamedName] | ||
| statusField, inStatus := r.StatusFields[renamedName] | ||
| switch { | ||
| case inSpec: | ||
| memberPath = cfg.PrefixConfig.SpecField | ||
| targetField = specField | ||
| case inStatus: | ||
| memberPath = cfg.PrefixConfig.StatusField | ||
| targetField = statusField | ||
| case isPrimaryIdentifier: | ||
| panic("Primary identifier field '" + memberName + "' in operation '" + op.Name + "' cannot be found in either spec or status.") | ||
| default: | ||
| continue | ||
| } | ||
|
|
||
| targetVarPath := fmt.Sprintf("%s%s", targetVarName, memberPath) |
There was a problem hiding this comment.
Could use getSanitizedMemberPath here
There was a problem hiding this comment.
Hmm, I think there would still be roughly the same number of LoC even if I use getSanitizedMemberPath, here. Mainly because I need a reference to the *Field - as its required for the setResourceForScalar parameters. Although I'm essentially doing the same logic, just with an extra return value.
I might refactor getSanitizedMemberPath to return the field as well, but not in this PR.
|
|
||
| // Still haven't determined the identifier? Panic | ||
| if primaryIdentifier == "" { | ||
| switch len(identifiers) { |
There was a problem hiding this comment.
So clean. I love this!
RedbackThomson
added
the
kind/enhancement
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jaypipes, RedbackThomson The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Closes aws-controllers-k8s/community#909
Description of changes:
The SageMaker
ModelPackage(in some cases) needs to be described using the resource ARN, even though the spec has aModelPackageNameand theDescribeModelPackageinput shape has aModelPackageNamefield. This pull requests allows the service teams to set theprimary_identifier_field_nameas"ARN", to override the default generator logic in order to set theACKResourceMetadata.ARNfield to beidentifier.ARN.Also refactored the identifier code to use the new
FindIdentifiersInShapecommon method and to use thesetResourceForScalarmethod.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.