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.

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

Compare tags using common tag type #399

Merged
merged 1 commit into from
Feb 9, 2023
Merged

Compare tags using common tag type #399

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
149 changes: 110 additions & 39 deletions pkg/generate/code/compare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,38 +42,41 @@ import (
//
// For *scalar* Go types, the output Go code looks like this:
//
// if ackcompare.HasNilDifference(a.ko.Spec.GrantFullControl, b.ko.Spec.GrantFullControl) {
// delta.Add("Spec.GrantFullControl", a.ko.Spec.GrantFullControl, b.ko.Spec.GrantFullControl)
// } else if a.ko.Spec.GrantFullControl != nil && b.ko.Spec.GrantFullControl != nil {
// if *a.ko.Spec.GrantFullControl != *b.ko.Spec.GrantFullControl {
// delta.Add("Spec.GrantFullControl", a.ko.Spec.GrantFullControl, b.ko.Spec.GrantFullControl)
// }
// }
// if ackcompare.HasNilDifference(a.ko.Spec.GrantFullControl, b.ko.Spec.GrantFullControl) {
// delta.Add("Spec.GrantFullControl", a.ko.Spec.GrantFullControl, b.ko.Spec.GrantFullControl)
// } else if a.ko.Spec.GrantFullControl != nil && b.ko.Spec.GrantFullControl != nil {
//
// if *a.ko.Spec.GrantFullControl != *b.ko.Spec.GrantFullControl {
// delta.Add("Spec.GrantFullControl", a.ko.Spec.GrantFullControl, b.ko.Spec.GrantFullControl)
// }
// }
//
// For *struct* Go types, the output Go code looks like this (note that it is a
// simple recursive-descent output of all the struct's fields...):
//
// if ackcompare.HasNilDifference(a.ko.Spec.CreateBucketConfiguration, b.ko.Spec.CreateBucketConfiguration) {
// delta.Add("Spec.CreateBucketConfiguration", a.ko.Spec.CreateBucketConfiguration, b.ko.Spec.CreateBucketConfiguration)
// } else if a.ko.Spec.CreateBucketConfiguration != nil && b.ko.Spec.CreateBucketConfiguration != nil {
// if ackcompare.HasNilDifference(a.ko.Spec.CreateBucketConfiguration.LocationConstraint, b.ko.Spec.CreateBucketConfiguration.LocationConstraint) {
// delta.Add("Spec.CreateBucketConfiguration.LocationConstraint", a.ko.Spec.CreateBucketConfiguration.LocationConstraint, b.ko.Spec.CreateBucketConfiguration.LocationConstraint)
// } else if a.ko.Spec.CreateBucketConfiguration.LocationConstraint != nil && b.ko.Spec.CreateBucketConfiguration.LocationConstraint != nil {
// if *a.ko.Spec.CreateBucketConfiguration.LocationConstraint != *b.ko.Spec.CreateBucketConfiguration.LocationConstraint {
// delta.Add("Spec.CreateBucketConfiguration.LocationConstraint", a.ko.Spec.CreateBucketConfiguration.LocationConstraint, b.ko.Spec.CreateBucketConfiguration.LocationConstraint)
// }
// }
// }
// if ackcompare.HasNilDifference(a.ko.Spec.CreateBucketConfiguration, b.ko.Spec.CreateBucketConfiguration) {
// delta.Add("Spec.CreateBucketConfiguration", a.ko.Spec.CreateBucketConfiguration, b.ko.Spec.CreateBucketConfiguration)
// } else if a.ko.Spec.CreateBucketConfiguration != nil && b.ko.Spec.CreateBucketConfiguration != nil {
//
// if ackcompare.HasNilDifference(a.ko.Spec.CreateBucketConfiguration.LocationConstraint, b.ko.Spec.CreateBucketConfiguration.LocationConstraint) {
// delta.Add("Spec.CreateBucketConfiguration.LocationConstraint", a.ko.Spec.CreateBucketConfiguration.LocationConstraint, b.ko.Spec.CreateBucketConfiguration.LocationConstraint)
// } else if a.ko.Spec.CreateBucketConfiguration.LocationConstraint != nil && b.ko.Spec.CreateBucketConfiguration.LocationConstraint != nil {
// if *a.ko.Spec.CreateBucketConfiguration.LocationConstraint != *b.ko.Spec.CreateBucketConfiguration.LocationConstraint {
// delta.Add("Spec.CreateBucketConfiguration.LocationConstraint", a.ko.Spec.CreateBucketConfiguration.LocationConstraint, b.ko.Spec.CreateBucketConfiguration.LocationConstraint)
// }
// }
// }
//
// For *slice of strings* Go types, the output Go code looks like this:
//
// if ackcompare.HasNilDifference(a.ko.Spec.AllowedPublishers, b.ko.Spec.AllowedPublishers) {
// delta.Add("Spec.AllowedPublishers", a.ko.Spec.AllowedPublishers, b.ko.Spec.AllowedPublishers)
// } else if a.ko.Spec.AllowedPublishers != nil && b.ko.Spec.AllowedPublishers != nil {
// if !ackcompare.SliceStringPEqual(a.ko.Spec.AllowedPublishers.SigningProfileVersionARNs, b.ko.Spec.AllowedPublishers.SigningProfileVersionARNs) {
// delta.Add("Spec.AllowedPublishers.SigningProfileVersionARNs", a.ko.Spec.AllowedPublishers.SigningProfileVersionARNs, b.ko.Spec.AllowedPublishers.SigningProfileVersionARNs)
// }
// }
// if ackcompare.HasNilDifference(a.ko.Spec.AllowedPublishers, b.ko.Spec.AllowedPublishers) {
// delta.Add("Spec.AllowedPublishers", a.ko.Spec.AllowedPublishers, b.ko.Spec.AllowedPublishers)
// } else if a.ko.Spec.AllowedPublishers != nil && b.ko.Spec.AllowedPublishers != nil {
//
// if !ackcompare.SliceStringPEqual(a.ko.Spec.AllowedPublishers.SigningProfileVersionARNs, b.ko.Spec.AllowedPublishers.SigningProfileVersionARNs) {
// delta.Add("Spec.AllowedPublishers.SigningProfileVersionARNs", a.ko.Spec.AllowedPublishers.SigningProfileVersionARNs, b.ko.Spec.AllowedPublishers.SigningProfileVersionARNs)
// }
// }
func CompareResource(
cfg *ackgenconfig.Config,
r *model.CRD,
Expand All @@ -96,6 +99,11 @@ func CompareResource(

resConfig := cfg.GetResourceConfig(r.Names.Camel)

tagField, err := r.GetTagField()
if err != nil {
panic(err)
}

// We need a deterministic order to traverse our top-level fields...
specFieldNames := []string{}
for fieldName := range r.SpecFields {
Expand Down Expand Up @@ -144,10 +152,18 @@ func CompareResource(
out += fmt.Sprintf("%s}\n", indent)
continue
}

// Use a special comparison model for tags, since they need to be
// converted into the common ACK tag type before doing a map delta
if tagField != nil && specField == tagField {
out += compareTags(deltaVarName, firstResAdaptedVarName, secondResAdaptedVarName, fieldPath, indentLevel)
continue
}

memberShapeRef := specField.ShapeRef
memberShape := memberShapeRef.Shape

// if ackcompare.HasNilDifference(a.ko.Spec.Name, b.ko.Spec.Name == nil) {
// if ackcompare.HasNilDifference(a.ko.Spec.Name, b.ko.Spec.Name) {
// delta.Add("Spec.Name", a.ko.Spec.Name, b.ko.Spec.Name)
// }
nilCode := compareNil(
Expand Down Expand Up @@ -238,9 +254,9 @@ func CompareResource(
//
// Output code will look something like this:
//
// if ackcompare.HasNilDifferenceStringP(a.ko.Spec.Name, b.ko.Spec.Name == nil) {
// delta.Add("Spec.Name", a.ko.Spec.Name, b.ko.Spec.Name)
// }
// if ackcompare.HasNilDifferenceStringP(a.ko.Spec.Name, b.ko.Spec.Name == nil) {
// delta.Add("Spec.Name", a.ko.Spec.Name, b.ko.Spec.Name)
// }
func compareNil(
// struct informing code generator how to compare the field values
compareConfig *ackgenconfig.CompareFieldConfig,
Expand Down Expand Up @@ -302,9 +318,9 @@ func compareNil(
//
// Output code will look something like this:
//
// if *a.ko.Spec.Name != *b.ko.Spec.Name) {
// delta.Add("Spec.Name", a.ko.Spec.Name, b.ko.Spec.Name)
// }
// if *a.ko.Spec.Name != *b.ko.Spec.Name) {
// delta.Add("Spec.Name", a.ko.Spec.Name, b.ko.Spec.Name)
// }
func compareScalar(
// struct informing code generator how to compare the field values
compareConfig *ackgenconfig.CompareFieldConfig,
Expand Down Expand Up @@ -372,9 +388,9 @@ func compareScalar(
//
// Output code will look something like this:
//
// if !ackcompare.MapStringStringPEqual(a.ko.Spec.Tags, b.ko.Spec.Tags) {
// delta.Add("Spec.Tags", a.ko.Spec.Tags, b.ko.Spec.Tags)
// }
// if !ackcompare.MapStringStringPEqual(a.ko.Spec.Tags, b.ko.Spec.Tags) {
// delta.Add("Spec.Tags", a.ko.Spec.Tags, b.ko.Spec.Tags)
// }
func compareMap(
cfg *ackgenconfig.Config,
r *model.CRD,
Expand Down Expand Up @@ -449,9 +465,9 @@ func compareMap(
//
// Output code will look something like this:
//
// if !ackcompare.SliceStringPEqual(a.ko.Spec.SecurityGroupIDs, b.ko.Spec.SecurityGroupIDs) {
// delta.Add("Spec.SecurityGroupIDs", a.ko.Spec.SecurityGroupIDs, b.ko.Spec.SecurityGroupIDs)
// }
// if !ackcompare.SliceStringPEqual(a.ko.Spec.SecurityGroupIDs, b.ko.Spec.SecurityGroupIDs) {
// delta.Add("Spec.SecurityGroupIDs", a.ko.Spec.SecurityGroupIDs, b.ko.Spec.SecurityGroupIDs)
// }
func compareSlice(
cfg *ackgenconfig.Config,
r *model.CRD,
Expand Down Expand Up @@ -517,6 +533,46 @@ func compareSlice(
return out
}

// compareTags outputs Go code that compares two slices of tags from two
// resource fields by first converting them to the common ACK tag type and then
// using a map comparison. If there is a difference, adds the difference to a
// variable representing an `ackcompare.Delta`.
//
// Output code will look something like this:
//
// if !ackcompare.MapStringStringEqual(ToACKTags(a.ko.Spec.Tags), ToACKTags(b.ko.Spec.Tags)) {
// delta.Add("Spec.Tags", a.ko.Spec.Tags, b.ko.Spec.Tags)
// }
func compareTags(
// String representing the name of the variable that is of type
// `*ackcompare.Delta`. We will generate Go code that calls the `Add()`
// method of this variable when differences between fields are detected.
deltaVarName string,
// String representing the name of the variable that represents the first
// CR under comparison. This will typically be something like
// "a.ko.Spec.Name". See `templates/pkg/resource/delta.go.tpl`.
firstResVarName string,
// String representing the name of the variable that represents the second
// CR under comparison. This will typically be something like
// "b.ko.Spec.Name". See `templates/pkg/resource/delta.go.tpl`.
secondResVarName string,
// String indicating the current field path being evaluated, e.g.
// "Author.Name". This does not include the top-level Spec or Status
// struct.
fieldPath string,
// Number of levels of indentation to use
indentLevel int,
) string {
out := ""
indent := strings.Repeat("\t", indentLevel)

out += fmt.Sprintf("%sif !ackcompare.MapStringStringEqual(ToACKTags(%s), ToACKTags(%s)) {\n", indent, firstResVarName, secondResVarName)
out += fmt.Sprintf("%s\t%s.Add(\"%s\", %s, %s)\n", indent, deltaVarName, fieldPath, firstResVarName, secondResVarName)
out += fmt.Sprintf("%s}\n", indent)

return out
}

// CompareStruct outputs Go code that compares two struct values from two
// resource fields and, if there is a difference, adds the difference to a
// variable representing an `ackcompare.Delta`.
Expand Down Expand Up @@ -548,6 +604,11 @@ func CompareStruct(
) string {
out := ""

tagField, err := r.GetTagField()
if err != nil {
panic(err)
}

fieldConfigs := cfg.GetFieldConfigs(r.Names.Original)

for _, memberName := range shape.MemberNames() {
Expand All @@ -569,7 +630,9 @@ func CompareStruct(
// memberFieldPath contains the field path along with the prefix cfg.PrefixConfig.SpecField + "." hence we
// would need to substring to exclude cfg.PrefixConfig.SpecField + "." to get correct field config.
specFieldLen := len(strings.TrimPrefix(cfg.PrefixConfig.SpecField, "."))
fieldConfig := fieldConfigs[memberFieldPath[specFieldLen+1:len(memberFieldPath)]]
trimmedFieldPath := memberFieldPath[specFieldLen+1:]

fieldConfig := fieldConfigs[trimmedFieldPath]
if fieldConfig != nil {
compareConfig = fieldConfig.Compare
}
Expand All @@ -580,6 +643,13 @@ func CompareStruct(

memberShape := memberShapeRef.Shape

// Use a special comparison model for tags, since they need to be
// converted into the common ACK tag type before doing a map delta
if tagField != nil && tagField.Path == trimmedFieldPath {
out += compareTags(deltaVarName, firstResAdaptedVarName, secondResAdaptedVarName, fieldPath, indentLevel)
continue
}

// if ackcompare.HasNilDifference(a.ko.Spec.Name, b.ko.Spec.Name == nil) {
// delta.Add("Spec.Name", a.ko.Spec.Name, b.ko.Spec.Name)
// }
Expand All @@ -601,6 +671,7 @@ func CompareStruct(
)
indentLevel++
}

switch memberShape.Type {
case "structure":
// Recurse through all the struct's fields and subfields, building
Expand Down
17 changes: 10 additions & 7 deletions pkg/generate/code/compare_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,13 @@ func TestCompareResource_S3_Bucket(t *testing.T) {
delta.Add("Spec.ObjectLockEnabledForBucket", a.ko.Spec.ObjectLockEnabledForBucket, b.ko.Spec.ObjectLockEnabledForBucket)
}
}
if ackcompare.HasNilDifference(a.ko.Spec.Tagging, b.ko.Spec.Tagging) {
delta.Add("Spec.Tagging", a.ko.Spec.Tagging, b.ko.Spec.Tagging)
} else if a.ko.Spec.Tagging != nil && b.ko.Spec.Tagging != nil {
if !ackcompare.MapStringStringEqual(ToACKTags(a.ko.Spec.Tagging.TagSet), ToACKTags(b.ko.Spec.Tagging.TagSet)) {
delta.Add("Spec.Tagging", a.ko.Spec.Tagging.TagSet, b.ko.Spec.Tagging.TagSet)
}
}
`
assert.Equal(
expected,
Expand Down Expand Up @@ -333,12 +340,8 @@ func TestCompareResource_Lambda_Function(t *testing.T) {
delta.Add("Spec.Runtime", a.ko.Spec.Runtime, b.ko.Spec.Runtime)
}
}
if ackcompare.HasNilDifference(a.ko.Spec.Tags, b.ko.Spec.Tags) {
if !ackcompare.MapStringStringEqual(ToACKTags(a.ko.Spec.Tags), ToACKTags(b.ko.Spec.Tags)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In what package namespace is ToACKTags?

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 part of the generated code within each resource https://github.com/aws-controllers-k8s/ec2-controller/blob/main/pkg/resource/vpc/tags.go

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, duh, yeah I forgot about that. I thought for a second it was in runtime's pkg/tags

delta.Add("Spec.Tags", a.ko.Spec.Tags, b.ko.Spec.Tags)
} else if a.ko.Spec.Tags != nil && b.ko.Spec.Tags != nil {
if !ackcompare.MapStringStringPEqual(a.ko.Spec.Tags, b.ko.Spec.Tags) {
delta.Add("Spec.Tags", a.ko.Spec.Tags, b.ko.Spec.Tags)
}
}
if ackcompare.HasNilDifference(a.ko.Spec.Timeout, b.ko.Spec.Timeout) {
delta.Add("Spec.Timeout", a.ko.Spec.Timeout, b.ko.Spec.Timeout)
Expand Down Expand Up @@ -490,7 +493,7 @@ func TestCompareResource_IAM_OIDC_URL(t *testing.T) {
if !ackcompare.SliceStringPEqual(a.ko.Spec.ClientIDList, b.ko.Spec.ClientIDList) {
delta.Add("Spec.ClientIDList", a.ko.Spec.ClientIDList, b.ko.Spec.ClientIDList)
}
if !reflect.DeepEqual(a.ko.Spec.Tags, b.ko.Spec.Tags) {
if !ackcompare.MapStringStringEqual(ToACKTags(a.ko.Spec.Tags), ToACKTags(b.ko.Spec.Tags)) {
delta.Add("Spec.Tags", a.ko.Spec.Tags, b.ko.Spec.Tags)
}
if !ackcompare.SliceStringPEqual(a.ko.Spec.ThumbprintList, b.ko.Spec.ThumbprintList) {
Expand Down Expand Up @@ -539,7 +542,7 @@ func TestCompareResource_MemoryDB_User(t *testing.T) {
delta.Add("Spec.Name", a.ko.Spec.Name, b.ko.Spec.Name)
}
}
if !reflect.DeepEqual(a.ko.Spec.Tags, b.ko.Spec.Tags) {
if !ackcompare.MapStringStringEqual(ToACKTags(a.ko.Spec.Tags), ToACKTags(b.ko.Spec.Tags)) {
delta.Add("Spec.Tags", a.ko.Spec.Tags, b.ko.Spec.Tags)
}
`
Expand Down
1 change: 1 addition & 0 deletions pkg/model/model_s3_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ func TestS3_Bucket(t *testing.T) {
// ListBuckets API call...
"Name",
"ObjectLockEnabledForBucket",
"Tagging",
}
assert.Equal(expSpecFieldCamel, attrCamelNames(specFields))

Expand Down
3 changes: 3 additions & 0 deletions pkg/testdata/models/apis/lambda/0000-00-00/generator.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,6 @@ resources:
in:
- 1
- 2
CodeSigningConfig:
tags:
ignore: true
Copy link
Contributor

Choose a reason for hiding this comment

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

For my own edification: why is this necessary here for Lambda.CodeSigningConfig but not for other applicable test cases like MemoryDB.User?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CodeSigningConfig doesn't support tags in its create call - https://docs.aws.amazon.com/sdk-for-go/api/service/lambda/#CreateCodeSigningConfigInput - and for the purposes of these tests we aren't adding the custom field to support it

8 changes: 7 additions & 1 deletion pkg/testdata/models/apis/s3/0000-00-00/generator.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ resources:
list_operation:
match_fields:
- Name
tags:
path: Tagging.TagSet
fields:
ACL:
# This is to test the ackcompare field ignore functionality. This
Expand All @@ -27,4 +29,8 @@ resources:
Logging:
from:
operation: PutBucketLogging
path: BucketLoggingStatus
path: BucketLoggingStatus
Tagging:
from:
operation: PutBucketTagging
path: Tagging
2 changes: 2 additions & 0 deletions templates/pkg/resource/delta.go.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@ import (
"reflect"

ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
acktags "github.com/aws-controllers-k8s/runtime/pkg/tags"
)

// Hack to avoid import errors during build...
var (
_ = &bytes.Buffer{}
_ = &reflect.Method{}
_ = &acktags.Tags{}
)

// newResourceDelta returns a new `ackcompare.Delta` used to compare two
Expand Down