Skip to content

feat: return error on missing required fields during adoption #613

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: return error on missing required fields during adoption #613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
70 changes: 39 additions & 31 deletions pkg/generate/code/set_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,10 +843,14 @@ func identifierNameOrIDGuardConstructor(
// the required field for a read, or returning an error here
// and returns a `MissingNameIdentifier` error:
//
// if fields[${requiredField}] == "" {
// return ackerrors.MissingNameIdentifier
// f0, ok := fields[${requiredField}]
// if !ok {
// return ackerrors.NewTerminalError(fmt.Errorf("required field missing: ${requiredField}"))
// }
func requiredFieldGuardContructor(
// requiredFieldVarName is the variable where the requiredField value
// will be stored
requiredFieldVarName string,
// String representing the fields map that contains the required
// fields for adoption
sourceVarName string,
Expand All @@ -856,7 +860,7 @@ func requiredFieldGuardContructor(
indentLevel int,
) string {
indent := strings.Repeat("\t", indentLevel)
out := fmt.Sprintf("%stmp, ok := %s[\"%s\"]\n", indent, sourceVarName, requiredField)
out := fmt.Sprintf("%s%s, ok := %s[\"%s\"]\n", indent, requiredFieldVarName, sourceVarName, requiredField)
out += fmt.Sprintf("%sif !ok {\n", indent)
out += fmt.Sprintf("%s\treturn ackerrors.NewTerminalError(fmt.Errorf(\"required field missing: %s\"))\n", indent, requiredField)
out += fmt.Sprintf("%s}\n", indent)
Expand Down Expand Up @@ -1258,30 +1262,30 @@ func SetResourceIdentifiers(
//
// ```
//
// tmp, ok := field["brokerID"]
// primaryKey, ok := field["brokerID"]
// if !ok {
// return ackerrors.MissingNameIdentifier
Copy link
Member

Choose a reason for hiding this comment

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

how about return ackerrors.MissingNameIdentifier(fieldNames...)

Copy link
Member

Choose a reason for hiding this comment

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

So for a longtime this wasn't considered a terminal error?

Copy link
Member Author

Choose a reason for hiding this comment

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

MissingNameIdentifier is an error introduced by SetIdentifiers (Adopted CRD). It was mainly used when NameOrID was nil.
This error message fix was introduced here

// return ackerrors.NewTerminalError(fmt.Errorf("required field missing: brokerID"))
// }
// r.ko.Status.BrokerID = &tmp
// r.ko.Status.BrokerID = &primaryKey
//
// ```
//
// An example of code with additional keys:
//
// ```
//
// tmp, ok := field["resourceID"]
// if !ok {
// return ackerrors.MissingNameIdentifier
// }
//
// r.ko.Spec.ResourceID = &tmp
// primaryKey, ok := field["resourceID"]
// if !ok {
// return ackerrors.NewTerminalError(fmt.Errorf("required field missing: resourceID"))
// }
//
// f0, f0ok := fields["scalableDimension"]
// r.ko.Spec.ResourceID = &primaryKey
//
// if f0ok {
// r.ko.Spec.ScalableDimension = &f0
// }
// f0, ok := fields["scalableDimension"]
// if !ok {
// return ackerrors.NewTerminalError(fmt.Errorf("required field missing: scalableDimension"))
// }
// r.ko.Spec.ScalableDimension = &f0
//
// f1, f1ok := fields["serviceNamespace"]
//
Expand All @@ -1295,17 +1299,17 @@ func SetResourceIdentifiers(
// ```
//
// tmpArn, ok := field["arn"]
// if !ok {
// return ackerrors.MissingNameIdentifier
// if !ok {
// return ackerrors.NewTerminalError(fmt.Errorf("required field missing: arn"))
// }
// if r.ko.Status.ACKResourceMetadata == nil {
// r.ko.Status.ACKResourceMetadata = &ackv1alpha1.ResourceMetadata{}
// }
// arn := ackv1alpha1.AWSResourceName(tmp)
//
// r.ko.Status.ACKResourceMetadata.ARN = &arn
// r.ko.Status.ACKResourceMetadata.ARN = &arn
//
// f0, f0ok := fields["modelPackageName"]
// f0, f0ok := fields["modelPackageName"]
//
// if f0ok {
// r.ko.Spec.ModelPackageName = &f0
Expand Down Expand Up @@ -1355,10 +1359,10 @@ func PopulateResourceFromAnnotation(
out := "\n"
// Check if the CRD defines the primary keys
primaryKeyConditionalOut := "\n"
primaryKeyConditionalOut += requiredFieldGuardContructor(sourceVarName, "arn", indentLevel)
primaryKeyConditionalOut += requiredFieldGuardContructor("resourceARN", sourceVarName, "arn", indentLevel)
arnOut += ackResourceMetadataGuardConstructor(fmt.Sprintf("%s.Status", targetVarName), indentLevel)
arnOut += fmt.Sprintf(
"%sarn := ackv1alpha1.AWSResourceName(tmp)\n",
"%sarn := ackv1alpha1.AWSResourceName(resourceARN)\n",
indent,
)
arnOut += fmt.Sprintf(
Expand All @@ -1377,9 +1381,10 @@ func PopulateResourceFromAnnotation(
isPrimarySet := primaryField != nil
if isPrimarySet {
memberPath, _ := findFieldInCR(cfg, r, primaryField.Names.Original)
primaryKeyOut += requiredFieldGuardContructor(sourceVarName, primaryField.Names.CamelLower, indentLevel)
primaryKeyOut += requiredFieldGuardContructor("primaryKey", sourceVarName, primaryField.Names.CamelLower, indentLevel)
targetVarPath := fmt.Sprintf("%s%s", targetVarName, memberPath)
primaryKeyOut += setResourceIdentifierPrimaryIdentifierAnn(cfg, r,
"&primaryKey",
primaryField,
targetVarPath,
sourceVarName,
Expand Down Expand Up @@ -1451,18 +1456,19 @@ func PopulateResourceFromAnnotation(
switch targetField.ShapeRef.Shape.Type {
case "list", "structure", "map":
panic("primary identifier '" + targetField.Path + "' must be a scalar type since NameOrID is a string")
default:
break
Comment on lines -1454 to -1455
Copy link
Member

Choose a reason for hiding this comment

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

why? maybe a commment explaining

Copy link
Member Author

Choose a reason for hiding this comment

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

all break does is exit the switch case. since we're already at the end i thought it was unnecessary.
here's an example: https://go.dev/play/p/aFKpTVGC6BT

}

targetVarPath := fmt.Sprintf("%s%s", targetVarName, memberPath)
if isPrimaryIdentifier {
primaryKeyOut += requiredFieldGuardContructor(sourceVarName, targetField.Names.CamelLower, indentLevel)
if inputShape.IsRequired(memberName) || isPrimaryIdentifier {
requiredFieldVarName := fmt.Sprintf("f%d", memberIndex)
primaryKeyOut += requiredFieldGuardContructor(requiredFieldVarName, sourceVarName, targetField.Names.CamelLower, indentLevel)
primaryKeyOut += setResourceIdentifierPrimaryIdentifierAnn(cfg, r,
fmt.Sprintf("&%s", requiredFieldVarName),
targetField,
targetVarPath,
sourceVarName,
indentLevel)
indentLevel,
)
} else {
additionalKeyOut += setResourceIdentifierAdditionalKeyAnn(
cfg, r,
Expand All @@ -1471,7 +1477,8 @@ func PopulateResourceFromAnnotation(
targetVarPath,
sourceVarName,
names.New(fieldName).CamelLower,
indentLevel)
indentLevel,
)
}
}

Expand Down Expand Up @@ -1539,6 +1546,8 @@ func setResourceIdentifierPrimaryIdentifier(
func setResourceIdentifierPrimaryIdentifierAnn(
cfg *ackgenconfig.Config,
r *model.CRD,
// The variable used for required key
requiredFieldVarName string,
// The field that will be set on the target variable
targetField *model.Field,
// The variable name that we want to set a value to
Expand All @@ -1548,12 +1557,11 @@ func setResourceIdentifierPrimaryIdentifierAnn(
// Number of levels of indentation to use
indentLevel int,
) string {
adaptedMemberPath := fmt.Sprintf("&tmp")
qualifiedTargetVar := fmt.Sprintf("%s.%s", targetVarName, targetField.Path)

return setResourceForScalar(
qualifiedTargetVar,
adaptedMemberPath,
requiredFieldVarName,
targetField.ShapeRef,
indentLevel,
false,
Expand Down
22 changes: 11 additions & 11 deletions pkg/generate/code/set_resource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,6 @@ func TestSetResource_APIGWv2_Route_ReadOne(t *testing.T) {
)
}


func TestSetResource_SageMaker_Domain_ReadOne(t *testing.T) {
assert := assert.New(t)
require := require.New(t)
Expand Down Expand Up @@ -1800,11 +1799,11 @@ func TestSetResource_EKS_Cluster_PopulateResourceFromAnnotation(t *testing.T) {
require.NotNil(crd)

expected := `
tmp, ok := fields["name"]
f0, ok := fields["name"]
if !ok {
return ackerrors.NewTerminalError(fmt.Errorf("required field missing: name"))
}
r.ko.Spec.Name = &tmp
r.ko.Spec.Name = &f0
`
assert.Equal(
Expand All @@ -1823,15 +1822,15 @@ func TestSetResource_SageMaker_ModelPackage_PopulateResourceFromAnnotation(t *te
require.NotNil(crd)

expected := `
tmp, ok := identifier["arn"]
resourceARN, ok := identifier["arn"]
if !ok {
return ackerrors.NewTerminalError(fmt.Errorf("required field missing: arn"))
}
if r.ko.Status.ACKResourceMetadata == nil {
r.ko.Status.ACKResourceMetadata = &ackv1alpha1.ResourceMetadata{}
}
arn := ackv1alpha1.AWSResourceName(tmp)
arn := ackv1alpha1.AWSResourceName(resourceARN)
r.ko.Status.ACKResourceMetadata.ARN = &arn
`
assert.Equal(
Expand All @@ -1850,16 +1849,17 @@ func TestSetResource_APIGWV2_ApiMapping_PopulateResourceFromAnnotation(t *testin
require.NotNil(crd)

expected := `
tmp, ok := fields["apiMappingID"]
f0, ok := fields["apiMappingID"]
if !ok {
return ackerrors.NewTerminalError(fmt.Errorf("required field missing: apiMappingID"))
}
r.ko.Status.APIMappingID = &tmp
f1, f1ok := fields["domainName"]
if f1ok {
r.ko.Spec.DomainName = aws.String(f1)
r.ko.Status.APIMappingID = &f0
f1, ok := fields["domainName"]
if !ok {
return ackerrors.NewTerminalError(fmt.Errorf("required field missing: domainName"))
}
r.ko.Spec.DomainName = &f1
`
assert.Equal(
expected,
Expand Down