Skip to content
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

Allow override resource identifier to ARN #163

Merged
merged 4 commits into from
Aug 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 87 additions & 44 deletions pkg/generate/code/set_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -801,6 +801,19 @@ func SetResourceGetAttributes(
// r.ko.Spec.ServiceNamespace = f1
// }
// ```
// An example of code that uses the ARN:
//
// ```
// if r.ko.Status.ACKResourceMetadata == nil {
// r.ko.Status.ACKResourceMetadata = &ackv1alpha1.ResourceMetadata{}
// }
// r.ko.Status.ACKResourceMetadata.ARN = identifier.ARN
//
// f0, f0ok := identifier.AdditionalKeys["modelPackageName"]
// if f0ok {
// r.ko.Spec.ModelPackageName = &f0
// }
// ```
func SetResourceIdentifiers(
cfg *ackgenconfig.Config,
r *model.CRD,
Expand Down Expand Up @@ -832,15 +845,29 @@ func SetResourceIdentifiers(
return ""
}

primaryKeyOut := "\n"
arnOut := ""
primaryKeyOut := ""
primaryKeyConditionalOut := "\n"
additionalKeyOut := "\n"

indent := strings.Repeat("\t", indentLevel)

primaryKeyOut += fmt.Sprintf("%sif %s.NameOrID == \"\" {\n", indent, sourceVarName)
primaryKeyOut += fmt.Sprintf("%s\treturn ackerrors.MissingNameIdentifier\n", indent)
primaryKeyOut += fmt.Sprintf("%s}\n", indent)
// if identifier.NameOrID == "" {
// return ackerrors.MissingNameIdentifier
// }
primaryKeyConditionalOut += fmt.Sprintf("%sif %s.NameOrID == \"\" {\n", indent, sourceVarName)
primaryKeyConditionalOut += fmt.Sprintf("%s\treturn ackerrors.MissingNameIdentifier\n", indent)
primaryKeyConditionalOut += fmt.Sprintf("%s}\n", indent)

arnOut := "\n"
// if r.ko.Status.ACKResourceMetadata == nil {
// r.ko.Status.ACKResourceMetadata = &ackv1alpha1.ResourceMetadata{}
// }
// r.ko.Status.ACKResourceMetadata.ARN = identifier.ARN
arnOut += ackResourceMetadataGuardConstructor(fmt.Sprintf("%s.Status", targetVarName), indentLevel)
arnOut += fmt.Sprintf(
"%s%s.Status.ACKResourceMetadata.ARN = %s.ARN\n",
indent, targetVarName, sourceVarName,
)

primaryIdentifier := ""

Expand All @@ -852,33 +879,20 @@ func SetResourceIdentifiers(

// Determine the "primary identifier" based on the names of each field
if primaryIdentifier == "" {
primaryIdentifierLookup := []string{
"Name",
"Names",
r.Names.Original + "Name",
r.Names.Original + "Names",
r.Names.Original + "Id",
r.Names.Original + "Ids",
}

for _, memberName := range inputShape.MemberNames() {
if util.InStrings(memberName, primaryIdentifierLookup) {
if primaryIdentifier == "" {
primaryIdentifier = memberName
} else {
panic("Found multiple possible primary identifiers for " +
r.Names.Original + ". Set " +
"`primary_identifier_field_name` for the " + op.Name +
" operation in the generator config.")
}
}
}
identifiers := FindIdentifiersInShape(r, inputShape)

// Still haven't determined the identifier? Panic
if primaryIdentifier == "" {
switch len(identifiers) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v clean 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So clean. I love this!

case 0:
panic("Could not find primary identifier for " + r.Names.Original +
". Set `primary_identifier_field_name` for the " + op.Name +
" operation in the generator config.")
case 1:
primaryIdentifier = identifiers[0]
default:
panic("Found multiple possible primary identifiers for " +
r.Names.Original + ". Set " +
"`primary_identifier_field_name` for the " + op.Name +
" operation in the generator config.")
}
}

Expand All @@ -894,11 +908,11 @@ func SetResourceIdentifiers(
}

memberShapeRef, _ := inputShape.MemberRefs[memberName]
memberShape := memberShapeRef.Shape
sourceMemberShape := memberShapeRef.Shape

// Only strings are currently accepted as valid inputs for
// additional key fields
if memberShape.Type != "string" {
if sourceMemberShape.Type != "string" {
continue
}

Expand All @@ -908,40 +922,54 @@ func SetResourceIdentifiers(
}

if r.IsPrimaryARNField(memberName) {
// r.ko.Status.ACKResourceMetadata.ARN = identifier.ARN
arnOut += fmt.Sprintf(
"\n%s%s.Status.ACKResourceMetadata.ARN = %s.ARN\n",
indent, targetVarName, sourceVarName,
)
continue

}

// Check that the field has potentially been renamed
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)
Comment on lines 929 to +954
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use getSanitizedMemberPath here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if isPrimaryIdentifier {
// r.ko.Status.BrokerID = identifier.NameOrID
primaryKeyOut += fmt.Sprintf("%s%s%s.%s = &%s.NameOrID\n", indent, targetVarName, memberPath, cleanMemberName, sourceVarName)
// r.ko.Status.BrokerID = &identifier.NameOrID
adaptedMemberPath := fmt.Sprintf("&%s.NameOrID", sourceVarName)
switch sourceMemberShape.Type {
case "list", "structure", "map":
// TODO(RedbackThomson): Add support for slices and maps
// in ReadMany operations
break
default:
primaryKeyOut += setResourceForScalar(
cfg, r,
targetField.Path,
targetVarPath,
adaptedMemberPath,
memberShapeRef,
indentLevel,
)
}
} else {
// f0, f0ok := identifier.AdditionalKeys["scalableDimension"]
// if f0ok {
Expand All @@ -955,18 +983,33 @@ func SetResourceIdentifiers(
// throwing an error accessible to the user
additionalKeyOut += fmt.Sprintf("%s%s, %sok := %s\n", indent, fieldIndexName, fieldIndexName, sourceAdaptedVarName)
additionalKeyOut += fmt.Sprintf("%sif %sok {\n", indent, fieldIndexName)
additionalKeyOut += fmt.Sprintf("%s\t%s%s.%s = &%s\n", indent, targetVarName, memberPath, cleanMemberName, fieldIndexName)

switch sourceMemberShape.Type {
case "list", "structure", "map":
// TODO(RedbackThomson): Add support for slices and maps
// in ReadMany operations
break
default:
additionalKeyOut += setResourceForScalar(
cfg, r,
targetField.Path,
targetVarPath,
fmt.Sprintf("&%s", fieldIndexName),
memberShapeRef,
indentLevel+1,
)
}
additionalKeyOut += fmt.Sprintf("%s}\n", indent)

additionalKeyCount++
}
}

// Only use at most one of ARN or nameOrID as primary identifier outputs
if arnOut != "" {
if primaryIdentifier == "ARN" || primaryKeyOut == "" {
return arnOut + additionalKeyOut
}
return primaryKeyOut + additionalKeyOut
return primaryKeyConditionalOut + primaryKeyOut + additionalKeyOut
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this method has grown and grown over time. Will break it out when I implement aws-controllers-k8s/community#874

}

// setResourceForContainer returns a string of Go code that sets the value of a
Expand Down
26 changes: 26 additions & 0 deletions pkg/generate/code/set_resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3092,3 +3092,29 @@ func TestSetResource_APIGWV2_ApiMapping_SetResourceIdentifiers(t *testing.T) {
code.SetResourceIdentifiers(crd.Config(), crd, "identifier", "r.ko", 1),
)
}

func TestSetResource_SageMaker_ModelPackage_SetResourceIdentifiers(t *testing.T) {
assert := assert.New(t)
require := require.New(t)

g := testutil.NewModelForService(t, "sagemaker")

crd := testutil.GetCRDByName(t, g, "ModelPackage")
require.NotNil(crd)

expected := `
if r.ko.Status.ACKResourceMetadata == nil {
r.ko.Status.ACKResourceMetadata = &ackv1alpha1.ResourceMetadata{}
}
r.ko.Status.ACKResourceMetadata.ARN = identifier.ARN

f0, f0ok := identifier.AdditionalKeys["modelPackageName"]
if f0ok {
r.ko.Spec.ModelPackageName = &f0
}
`
assert.Equal(
expected,
code.SetResourceIdentifiers(crd.Config(), crd, "identifier", "r.ko", 1),
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

5 changes: 4 additions & 1 deletion pkg/testdata/models/apis/sagemaker/0000-00-00/generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ resources:
Endpoint:
reconcile:
requeue_on_success_seconds: 10
operations:
DescribeModelPackage:
primary_identifier_field_name: ARN
ignore:
resource_names:
- Algorithm
Expand Down Expand Up @@ -50,7 +53,7 @@ ignore:
- Model
- ModelBiasJobDefinition
- ModelExplainabilityJobDefinition
- ModelPackage
# - ModelPackage
# ModelPackageGroup
- ModelQualityJobDefinition
- MonitoringSchedule
Expand Down