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

feat(rds): instance + cluster allowMajorVersionUpgrade #1598

Conversation

wotolom
Copy link
Contributor

@wotolom wotolom commented Dec 9, 2022

Description of your changes

to both DBInstance and DBCluster

  • add field/flag allowMajorVersionUpgrade (+ special update handling needed for DBCluster)
  • make condition.status partly include AWS resource status (willing to revert parts here, if considered too redundant)
  • implement controllers updating VPCSecurityGroupIDs and DBParameterGroupName/DBClusterParameterGroupName

Fixes #1330

I have:

  • Read and followed Crossplane's [contribution process].
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

manual tests (DBInstance; DBCluster with DBInstance)

examples/rds/db-aurora-cluster.yaml Show resolved Hide resolved
@@ -424,6 +446,53 @@ func compareTimeRanges(format string, expectedWindow *string, actualWindow *stri
return false, nil
}

func compareVPCSecurityGroupIDs(cr *svcapitypes.DBInstance, out *svcsdk.DBInstance) bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The return value is easy to misunderstand. Maybe change it to isEqualVPCSecurityGroup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice to hear. I was initially unsure if or how I should use this more commonly used return logic in this existing resource.
Hopefully the newly committed change compromises not to much.

pkg/controller/rds/dbinstance/setup.go Outdated Show resolved Hide resolved
@wotolom wotolom force-pushed the rdsinstane-allow-major-version-upgrade branch from 8c6cbb4 to 3131a61 Compare December 14, 2022 10:37
@wotolom

This comment was marked as outdated.

@wotolom wotolom force-pushed the rdsinstane-allow-major-version-upgrade branch from 48f7d8d to dd12a90 Compare December 19, 2022 17:19
@wotolom wotolom requested a review from MisterMX December 19, 2022 17:41
pkg/controller/rds/dbinstance/setup.go Outdated Show resolved Hide resolved
pkg/controller/rds/dbinstance/setup.go Show resolved Hide resolved
@wotolom wotolom force-pushed the rdsinstane-allow-major-version-upgrade branch from dd12a90 to 9fc98b3 Compare December 20, 2022 09:31
@wotolom wotolom requested a review from MisterMX December 20, 2022 09:42
@MisterMX MisterMX merged commit b59bf42 into crossplane-contrib:master Dec 20, 2022
@afirth
Copy link

afirth commented Dec 20, 2022

Thank you!!

@wotolom wotolom deleted the rdsinstane-allow-major-version-upgrade branch December 21, 2022 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rds.aws.crossplane.io/v1alpha1 - missing parameter AllowMajorVersionUpgrade - cannot upgrade
3 participants