Skip to content
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
16 changes: 5 additions & 11 deletions pkg/generate/code/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,6 @@ func FindPrimaryIdentifierFieldNames(
) (crField string, shapeField string) {
shape := op.InputRef.Shape

// Attempt to fetch the primary identifier override from the configuration
opConfig, ok := cfg.Operations[op.Name]
if ok {
shapeField = opConfig.PrimaryIdentifierFieldName
}

if shapeField == "" {
// For ReadOne, search for a direct identifier
if op == r.Ops.ReadOne {
Expand All @@ -151,8 +145,8 @@ func FindPrimaryIdentifierFieldNames(
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.")
"`is_primary_key` for the primary field in the " +
r.Names.Camel + " resource.")
}
} else {
// For ReadMany, search for pluralized identifiers
Expand All @@ -162,12 +156,12 @@ func FindPrimaryIdentifierFieldNames(
// Require override if still can't find any identifiers
if shapeField == "" {
panic("Could not find primary identifier for " + r.Names.Original +
". Set `primary_identifier_field_name` for the " + op.Name +
" operation in the generator config.")
". Set `is_primary_key` for the primary field in the " +
r.Names.Camel + " resource.")
}
}

if r.IsPrimaryARNField(shapeField) || shapeField == PrimaryIdentifierARNOverride {
if r.IsPrimaryARNField(shapeField) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ApplicationAutoscaling does not utilize this, so I have not verified this snippet, looks ok though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SageMaker controller uses it (for ModelPackage). We have unit tests that cover it.

return "", PrimaryIdentifierARNOverride
}

Expand Down
66 changes: 47 additions & 19 deletions pkg/generate/code/set_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -788,12 +788,14 @@ func SetResourceGetAttributes(
// that an ARN, ID or Name) and any other "additional keys" required for the AWS
// service to uniquely identify the object.
//
// The method will attempt to use the `ReadOne` operation, if present, otherwise
// will fall back to using `ReadMany`. If it detects the operation uses an ARN
// to identify the resource it will read it from the metadata status field.
// Otherwise it will use any fields with a matching name in the operation,
// pulling from spec or status - requiring that exactly one of those fields is
// marked as the "primary" identifier.
// The method will attempt to look for the field denoted with a value of true
// for `is_primary_key`, or will use the ARN if the resource has a value of true
// for `is_arn_primary_key`. Otherwise, the method will attempt to use the
// `ReadOne` operation, if present, falling back to using `ReadMany`.
// If it detects the operation uses an ARN to identify the resource it will read
// it from the metadata status field. Otherwise it will use any field with a
// name that matches the primary identifier from the operation, pulling from
// top-level spec or status fields.
//
// An example of code with no additional keys:
//
Expand Down Expand Up @@ -873,21 +875,42 @@ func SetResourceIdentifiers(
primaryKeyConditionalOut := "\n"
primaryKeyConditionalOut += identifierNameOrIDGuardConstructor(sourceVarName, indentLevel)

primaryCRField, primaryShapeField := FindPrimaryIdentifierFieldNames(cfg, r, op)
if primaryShapeField == PrimaryIdentifierARNOverride {
// if r.ko.Status.ACKResourceMetadata == nil {
// r.ko.Status.ACKResourceMetadata = &ackv1alpha1.ResourceMetadata{}
// }
// r.ko.Status.ACKResourceMetadata.ARN = identifier.ARN
arnOut := "\n"
arnOut += ackResourceMetadataGuardConstructor(fmt.Sprintf("%s.Status", targetVarName), indentLevel)
arnOut += fmt.Sprintf(
"%s%s.Status.ACKResourceMetadata.ARN = %s.ARN\n",
indent, targetVarName, sourceVarName,
)
// if r.ko.Status.ACKResourceMetadata == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's up with this commented out code ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a code sample for the generated code for the following block :)

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

// Check if the CRD defines the primary keys
if r.IsARNPrimaryKey() {
return arnOut
}
primaryField, err := r.GetPrimaryKeyField()
if err != nil {
panic(err)
}

var primaryCRField, primaryShapeField string
isPrimarySet := primaryField != nil
if isPrimarySet {
memberPath, _ := findFieldInCR(cfg, r, primaryField.Names.Original)
targetVarPath := fmt.Sprintf("%s%s", targetVarName, memberPath)
primaryKeyOut += setResourceIdentifierPrimaryIdentifier(cfg, r,
primaryField,
targetVarPath,
sourceVarName,
indentLevel)
} else {
primaryCRField, primaryShapeField = FindPrimaryIdentifierFieldNames(cfg, r, op)
if primaryShapeField == PrimaryIdentifierARNOverride {
return arnOut
}
}

paginatorFieldLookup := []string{
"NextToken",
Expand Down Expand Up @@ -924,6 +947,11 @@ func SetResourceIdentifiers(
op.Name, memberName,
)

// Check to see if we've already set the field as the primary identifier
if isPrimarySet && renamedName == primaryField.Names.Camel {
continue
}

isPrimaryIdentifier := memberName == primaryShapeField

searchField := ""
Expand All @@ -934,7 +962,7 @@ func SetResourceIdentifiers(
}

memberPath, targetField := findFieldInCR(cfg, r, searchField)
if targetField == nil {
if targetField == nil || (isPrimarySet && targetField == primaryField) {
continue
}

Expand Down
8 changes: 4 additions & 4 deletions pkg/generate/config/field.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,11 @@ type FieldConfig struct {
// Required indicates whether this field is a required member or not.
// This field is used to configure '+kubebuilder:validation:Required' on API object's members.
IsRequired *bool `json:"is_required,omitempty"`
// IsName indicates the field represents the name/string identifier field
// for the resource. This allows the generator config to override the
// default behaviour of considering a field called "Name" or
// IsPrimaryKey indicates the field represents the primary name/string
// identifier field for the resource. This allows the generator config to
// override the default behaviour of considering a field called "Name" or
// "{Resource}Name" or "{Resource}Id" as the "name field" for the resource.
IsName bool `json:"is_name"`
IsPrimaryKey bool `json:"is_primary_key"`
Copy link
Contributor

Choose a reason for hiding this comment

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

My local config has both is_name and is_primary_key since I had not looked at this code yet. But generator does not throw an error. We should have some validation in the future for extra fields too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I did a global search for all service controllers using is_name and I believe SageMaker is the only example of them. I also agree that the generator should at least warn teams if they have unspecified fields.

// IsOwnerAccountID indicates the field contains the AWS Account ID
// that owns the resource. This is a special field that we direct to
// storage in the common `Status.ACKResourceMetadata.OwnerAccountID` field.
Expand Down
6 changes: 1 addition & 5 deletions pkg/generate/config/operation.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (

awssdkmodel "github.com/aws/aws-sdk-go/private/model/api"

"github.com/aws-controllers-k8s/code-generator/pkg/util"
"github.com/aws-controllers-k8s/code-generator/pkg/util"
)

// StringArray is a type that can be represented in JSON as *either* a string
Expand All @@ -45,10 +45,6 @@ type OperationConfig struct {
// An example of this is `Put...` or `Register...` API operations not being correctly classified as `Create` op type
// OperationType []string `json:"operation_type"`
OperationType StringArray `json:"operation_type"`
// PrimaryIdentifierFieldName provides the name of the field that should be
// interpreted as the "primary" identifier field. This field will be used as
// the primary field for resource adoption.
PrimaryIdentifierFieldName string `json:"primary_identifier_field_name,omitempty"`
}

// IsIgnoredOperation returns true if Operation Name is configured to be ignored
Expand Down
3 changes: 3 additions & 0 deletions pkg/generate/config/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@ type ResourceConfig struct {
// Print contains instructions for the code generator to generate kubebuilder printcolumns
// marker comments.
Print *PrintConfig `json:"print,omitempty"`
// IsARNPrimaryKey determines whether the CRD uses the ARN as the primary
// identifier in the ReadOne operations.
IsARNPrimaryKey bool `json:"is_arn_primary_key"`
}

// HooksConfig instructs the code generator how to inject custom callback hooks
Expand Down
44 changes: 41 additions & 3 deletions pkg/model/crd.go
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,46 @@ func (r *CRD) HasImmutableFieldChanges() bool {
return false
}

// IsARNPrimaryKey returns true if the CRD uses its ARN as its primary key in
// ReadOne calls.
func (r *CRD) IsARNPrimaryKey() bool {
if r.cfg == nil {
return false
}
resGenConfig, found := r.cfg.Resources[r.Names.Original]
if !found {
return false
}
return resGenConfig.IsARNPrimaryKey
}

// GetPrimaryKeyField returns the field designated as the primary key, nil if
// none are specified or an error if multiple are designated.
func (r *CRD) GetPrimaryKeyField() (*Field, error) {
fConfigs := r.cfg.ResourceFields(r.Names.Original)

var primaryField *Field
for fieldName, fieldConfig := range fConfigs {
if !fieldConfig.IsPrimaryKey {
continue
}

// Multiple primary fields
if primaryField != nil {
return nil, fmt.Errorf("multiple fields are marked with is_primary_key")
}

fieldNames := names.New(fieldName)
fPath := fieldNames.Camel
var notFound bool
primaryField, notFound = r.Fields[fPath]
if !notFound {
return nil, fmt.Errorf("what the frick")
}
}
return primaryField, nil
}

// SetOutputCustomMethodName returns custom set output operation as *string for
// given operation on custom resource, if specified in generator config
func (r *CRD) SetOutputCustomMethodName(
Expand Down Expand Up @@ -536,14 +576,12 @@ func (r *CRD) GetCustomCheckRequiredFieldsMissingMethod(

// SpecIdentifierField returns the name of the "Name" or string identifier field
// in the Spec.
// This method does not guarantee that the identifier field returned is the
// primary identifier used in any of the `Read*` operations.
func (r *CRD) SpecIdentifierField() *string {
if r.cfg != nil {
rConfig, found := r.cfg.Resources[r.Names.Original]
if found {
for fName, fConfig := range rConfig.Fields {
if fConfig.IsName {
if fConfig.IsPrimaryKey {
return &fName
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ resources:
Repository:
fields:
Name:
is_name: true
is_primary_key: true
exceptions:
errors:
404:
Expand Down
12 changes: 7 additions & 5 deletions pkg/testdata/models/apis/rds/0000-00-00/generator.yaml
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
ignore:
shape_names:
- DBSecurityGroupMembershipList
operations:
DescribeDBInstances:
primary_identifier_field_name: DBInstanceIdentifier
DescribeDBSubnetGroups:
primary_identifier_field_name: DBSubnetGroupName
resources:
DBInstance:
fields:
DBInstanceIdentifier:
is_primary_key: true
DBSubnetGroup:
fields:
Name:
is_primary_key: true
renames:
operations:
DescribeDBSubnetGroups:
Expand Down
5 changes: 2 additions & 3 deletions pkg/testdata/models/apis/sagemaker/0000-00-00/generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,8 @@ resources:
Endpoint:
reconcile:
requeue_on_success_seconds: 10
operations:
DescribeModelPackage:
primary_identifier_field_name: ARN
ModelPackage:
is_arn_primary_key: true
ignore:
resource_names:
- Algorithm
Expand Down