Skip to content

Modify tags related functions and some test files to fix flaky assertion errors #51

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

Merged
merged 1 commit into from
Feb 6, 2023
Merged

Modify tags related functions and some test files to fix flaky assertion errors #51

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
8 changes: 4 additions & 4 deletions apis/v1alpha1/ack-generate-metadata.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
ack_generate_info:
build_date: "2023-01-10T21:58:28Z"
build_hash: 1b20baf45a0b73a11b296050322a384c705fa897
go_version: go1.17.5
version: v0.22.0
build_date: "2023-02-03T00:39:12Z"
build_hash: c6651c200ba136e5c7f50ad8be751fae060a38e6
go_version: go1.19
version: v0.22.0-1-gc6651c2
api_directory_checksum: ee32acc4d4a0ba7e2823dd20fdbe2c4ef1d9e0f4
api_version: v1alpha1
aws_sdk_go_version: v1.44.93
Expand Down
4 changes: 2 additions & 2 deletions apis/v1alpha1/subnet_group.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions apis/v1alpha1/types.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions helm/crds/memorydb.services.k8s.aws_subnetgroups.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ spec:
type: object
spec:
description: "SubnetGroupSpec defines the desired state of SubnetGroup.
\n Represents the output of one of the following operations: \n * CreateSubnetGroup
\n * UpdateSubnetGroup \n A subnet group is a collection of subnets
\n Represents the output of one of the following operations: \n - CreateSubnetGroup
\n - UpdateSubnetGroup \n A subnet group is a collection of subnets
(typically private) that you can designate for your clusters running
in an Amazon Virtual Private Cloud (VPC) environment."
properties:
Expand Down
30 changes: 25 additions & 5 deletions pkg/resource/acl/hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,13 @@ import (
"github.com/aws-controllers-k8s/runtime/pkg/requeue"
ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
ackutil "github.com/aws-controllers-k8s/runtime/pkg/util"
svcsdk "github.com/aws/aws-sdk-go/service/memorydb"

svcapitypes "github.com/aws-controllers-k8s/memorydb-controller/apis/v1alpha1"
svcsdk "github.com/aws/aws-sdk-go/service/memorydb"
)

var (
resourceStatusActive string = "active"
)

// validateACLNeedsUpdate this function's purpose is to requeue if the resource is currently unavailable
Expand All @@ -32,7 +36,7 @@ func (rm *resourceManager) validateACLNeedsUpdate(

// requeue if necessary
latestStatus := latest.ko.Status.Status
if latestStatus == nil || *latestStatus != "active" {
if latestStatus == nil || *latestStatus != resourceStatusActive {
return requeue.NeededAfter(
errors.New("ACL cannot be modified as its status is not 'active'."),
requeue.DefaultRequeueAfterDuration)
Expand All @@ -41,6 +45,14 @@ func (rm *resourceManager) validateACLNeedsUpdate(
return nil
}

// aclActive returns true when the status of the given ACL is set to `active`
func (rm *resourceManager) aclActive(
latest *resource,
) bool {
latestStatus := latest.ko.Status.Status
return latestStatus != nil && *latestStatus == resourceStatusActive
}

// getTags gets tags from given ParameterGroup.
func (rm *resourceManager) getTags(
ctx context.Context,
Expand Down Expand Up @@ -114,17 +126,23 @@ func computeTagsDelta(
latest []*svcapitypes.Tag,
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we are reusing the computeTagsDelta in every hooks.go file. We can reduce this duplication by moving this method into a package that is accessible by all resources. RDS puts theirs in pkg/utils/tags.go and then imports that module inside hooks.go - https://github.com/aws-controllers-k8s/rds-controller/blob/main/pkg/util/tags.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will create another PR to do it.

) (addedOrUpdated []*svcapitypes.Tag, removed []*string) {
var visitedIndexes []string
var hasSameKey bool

for _, latestElement := range latest {
hasSameKey = false
visitedIndexes = append(visitedIndexes, *latestElement.Key)
for _, desiredElement := range desired {
if equalStrings(latestElement.Key, desiredElement.Key) {
hasSameKey = true
if !equalStrings(latestElement.Value, desiredElement.Value) {
addedOrUpdated = append(addedOrUpdated, desiredElement)
}
continue
break
}
}
if hasSameKey {
continue
}
removed = append(removed, latestElement.Key)
}
for _, desiredElement := range desired {
Expand Down Expand Up @@ -163,7 +181,9 @@ func resourceTagsFromSDKTags(

func equalStrings(a, b *string) bool {
if a == nil {
return b == nil || *b == ""
return b == nil
} else if b == nil {
return false
}
return (*a == "" && b == nil) || *a == *b
return *a == *b
}
12 changes: 7 additions & 5 deletions pkg/resource/acl/sdk.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 23 additions & 6 deletions pkg/resource/cluster/hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,15 @@ import (
ackrequeue "github.com/aws-controllers-k8s/runtime/pkg/requeue"
ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
ackutil "github.com/aws-controllers-k8s/runtime/pkg/util"
svcsdk "github.com/aws/aws-sdk-go/service/memorydb"

svcapitypes "github.com/aws-controllers-k8s/memorydb-controller/apis/v1alpha1"
svcsdk "github.com/aws/aws-sdk-go/service/memorydb"
)

var (
condMsgCurrentlyDeleting = "cluster currently being deleted"
condMsgNoDeleteWhileUpdating = "cluster is being updated. cannot delete"
condMsgCurrentlyDeleting = "cluster currently being deleted"
condMsgNoDeleteWhileUpdating = "cluster is being updated. cannot delete"
Comment on lines +33 to +34
Copy link
Member

Choose a reason for hiding this comment

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

Can we make messages a bit consistent? cluster currently being updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two variables are not related to flaky tests. I didn't modify or create them in this PR, so I will mark it and change it in a new PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please. The english grammar for these two messages should be improved. cluster is currently being deleted and cluster is currently being updated, cannot delete are my suggestions

resourceStatusActive string = "active"
)

var (
Expand Down Expand Up @@ -264,6 +265,14 @@ func (rm *resourceManager) newMemoryDBClusterUploadPayload(
return res
}

// clusterActive returns true when the status of the given Cluster is set to `active`
func (rm *resourceManager) clusterActive(
latest *resource,
) bool {
latestStatus := latest.ko.Status.Status
return latestStatus != nil && *latestStatus == resourceStatusActive
}

// getTags gets tags from given ParameterGroup.
func (rm *resourceManager) getTags(
ctx context.Context,
Expand Down Expand Up @@ -337,17 +346,23 @@ func computeTagsDelta(
latest []*svcapitypes.Tag,
) (addedOrUpdated []*svcapitypes.Tag, removed []*string) {
var visitedIndexes []string
var hasSameKey bool

for _, latestElement := range latest {
hasSameKey = false
visitedIndexes = append(visitedIndexes, *latestElement.Key)
for _, desiredElement := range desired {
if equalStrings(latestElement.Key, desiredElement.Key) {
hasSameKey = true
if !equalStrings(latestElement.Value, desiredElement.Value) {
addedOrUpdated = append(addedOrUpdated, desiredElement)
}
continue
break
}
}
if hasSameKey {
continue
}
Comment on lines +363 to +365
Copy link
Member

Choose a reason for hiding this comment

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

I see that you wanna ignore duplicated keys +++

removed = append(removed, latestElement.Key)
}
for _, desiredElement := range desired {
Expand Down Expand Up @@ -386,7 +401,9 @@ func resourceTagsFromSDKTags(

func equalStrings(a, b *string) bool {
if a == nil {
return b == nil || *b == ""
return b == nil
} else if b == nil {
return false
}
return (*a == "" && b == nil) || *a == *b
return *a == *b
}
12 changes: 7 additions & 5 deletions pkg/resource/cluster/sdk.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

77 changes: 46 additions & 31 deletions pkg/resource/parameter_group/hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ import (
ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1"
ackerr "github.com/aws-controllers-k8s/runtime/pkg/errors"
ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
ackutil "github.com/aws-controllers-k8s/runtime/pkg/util"
svcsdk "github.com/aws/aws-sdk-go/service/memorydb"

svcapitypes "github.com/aws-controllers-k8s/memorydb-controller/apis/v1alpha1"

svcsdk "github.com/aws/aws-sdk-go/service/memorydb"
)

func (rm *resourceManager) setParameters(
Expand Down Expand Up @@ -173,13 +173,7 @@ func (rm *resourceManager) getTags(
if err != nil {
return nil, err
}
tags := make([]*svcapitypes.Tag, 0, len(resp.TagList))
for _, tag := range resp.TagList {
tags = append(tags, &svcapitypes.Tag{
Key: tag.Key,
Value: tag.Value,
})
}
tags := resourceTagsFromSDKTags(resp.TagList)
return tags, nil
}

Expand Down Expand Up @@ -235,34 +229,33 @@ func (rm *resourceManager) updateTags(
func computeTagsDelta(
desired []*svcapitypes.Tag,
latest []*svcapitypes.Tag,
) (added []*svcapitypes.Tag, removed []*string) {
toDelete := []*string{}
toAdd := []*svcapitypes.Tag{}
) (addedOrUpdated []*svcapitypes.Tag, removed []*string) {
var visitedIndexes []string
var hasSameKey bool

desiredTags := map[string]string{}
key := ""
value := ""
for _, tag := range desired {
if tag.Key != nil {
key = *tag.Key
value = ""
if tag.Value != nil {
value = *tag.Value
for _, latestElement := range latest {
hasSameKey = false
visitedIndexes = append(visitedIndexes, *latestElement.Key)
for _, desiredElement := range desired {
if equalStrings(latestElement.Key, desiredElement.Key) {
hasSameKey = true
if !equalStrings(latestElement.Value, desiredElement.Value) {
addedOrUpdated = append(addedOrUpdated, desiredElement)
}
break
}
desiredTags[key] = value
}
if hasSameKey {
continue
}
removed = append(removed, latestElement.Key)
}

for _, tag := range desired {
toAdd = append(toAdd, tag)
}
for _, tag := range latest {
_, ok := desiredTags[*tag.Key]
if !ok {
toDelete = append(toDelete, tag.Key)
for _, desiredElement := range desired {
if !ackutil.InStrings(*desiredElement.Key, visitedIndexes) {
addedOrUpdated = append(addedOrUpdated, desiredElement)
}
}
return toAdd, toDelete
return addedOrUpdated, removed
}

func sdkTagsFromResourceTags(
Expand All @@ -277,3 +270,25 @@ func sdkTagsFromResourceTags(
}
return tags
}

func resourceTagsFromSDKTags(
sdkTags []*svcsdk.Tag,
) []*svcapitypes.Tag {
tags := make([]*svcapitypes.Tag, len(sdkTags))
for i := range sdkTags {
tags[i] = &svcapitypes.Tag{
Key: sdkTags[i].Key,
Value: sdkTags[i].Value,
}
}
return tags
}

func equalStrings(a, b *string) bool {
if a == nil {
return b == nil
} else if b == nil {
return false
}
return *a == *b
}
Loading