Move DBSubnetGroup to v1beta1 #162

gauravgahlot · 2020-03-23T07:58:26Z

Description of your changes

Moved DBSubnetGroup implementation from v1alpha3 to v1beta1.
Added implementation for DBSubnetGroup update

Tested with the following:

apiVersion: database.aws.crossplane.io/v1beta1
kind: DBSubnetGroup
metadata:
  name: sample-dbsubnetgroup
  annotations:
    crossplane.io/external-name: managed-resource-dbsubnetgroup
spec:
  forProvider:
    description: EKS vpc to rds
    subnetIdRefs:
      - name: sample-subnet1
      - name: sample-subnet2
      - name: sample-subnet3
    tags:
      - key: name
        value: my-cool-dbsubnetgroup
  reclaimPolicy: Delete
  providerRef:
    name: aws-provider

Checklist

I have:

Run make reviewable to ensure this PR is ready for review.
Ensured this PR contains a neat, self-documenting set of commits.
Updated any relevant documentation, examples, or release notes.
Updated the dependencies in app.yaml to include any new role permissions.

pkg/controller/database/dbsubnetgroup/controller.go

apis/database/v1beta1/dbsubnetgroup_referencers.go

pkg/controller/database/dbsubnetgroup/controller.go

muvaf · 2020-03-25T17:26:06Z

pkg/controller/database/dbsubnetgroup/controller.go

 	if !ok {
 		return errors.New(errUnexpectedObject)
 	}

 	cr.Status.SetConditions(runtimev1alpha1.Deleting())

 	req := e.client.DeleteDBSubnetGroupRequest(&awsrds.DeleteDBSubnetGroupInput{
-		DBSubnetGroupName: aws.String(cr.Spec.DBSubnetGroupName),
+		DBSubnetGroupName: aws.String(cr.Spec.ForProvider.DBSubnetGroupName),


Suggested change

DBSubnetGroupName: aws.String(cr.Spec.ForProvider.DBSubnetGroupName),

DBSubnetGroupName: meta.GetExternalName(cr),

muvaf · 2020-03-25T17:27:42Z

pkg/controller/database/dbsubnetgroup/controller.go

 	if !ok {
 		return errors.New(errUnexpectedObject)
 	}

 	cr.Status.SetConditions(runtimev1alpha1.Deleting())

 	req := e.client.DeleteDBSubnetGroupRequest(&awsrds.DeleteDBSubnetGroupInput{
-		DBSubnetGroupName: aws.String(cr.Spec.DBSubnetGroupName),
+		DBSubnetGroupName: aws.String(cr.Spec.ForProvider.DBSubnetGroupName),
 	})

 	_, err := req.Send(ctx)


Suggested change

_, err := req.Send(ctx)

_, err := req.Send(ctx)

return errors.Wrap(resource.Ignore(rds.IsDBSubnetGroupNotFoundErr, err), errDelete)

resource.Ignore will skip that specific error by running the func you provide, which is rds.IsDBSubnetGroupNotFoundErr in this case.

pkg/controller/database/dbsubnetgroup/controller.go

apis/database/v1beta1/dbsubnetgroup_types.go

muvaf

This looks pretty good. I think we can merge it after the comments are addressed. I'd also suggest going over the commit list and consolidate some of them. For commit messages, I'm not sure whether we have a convention but I usually go with dbsubnetgroup: this commit does that.

apis/database/v1beta1/dbsubnetgroup_types.go

pkg/controller/database/dbsubnetgroup/controller.go

muvaf · 2020-04-01T20:25:30Z

pkg/clients/dbsubnetgroup/dbsubnetgroup.go

+}
+
+// IsDBSubnetGroupUpToDate checks whether there is a change in any of the modifiable fields.
+func IsDBSubnetGroupUpToDate(p v1beta1.DBSubnetGroupParameters, sg rds.DBSubnetGroup) (bool, error) {


There are only 2 fields that might need update since Modify call takes only those. So, it might be easier just to check DBSubnetGroupDescription and SubnetIDs fields and see whether they changed and return.

This comment assumed that we were not able to update the tags but that's actually possible later I realized. So, you need to check tags as well.

apis/database/v1beta1/dbsubnetgroup_types.go

muvaf · 2020-04-01T20:28:27Z

pkg/controller/database/dbsubnetgroup/controller.go

-		SubnetIds:                cr.Spec.SubnetIDs,
+		DBSubnetGroupDescription: aws.String(cr.Spec.ForProvider.DBSubnetGroupDescription),
+		DBSubnetGroupName:        aws.String(cr.Spec.ForProvider.DBSubnetGroupName),
+		SubnetIds:                cr.Spec.ForProvider.SubnetIDs,


If I recall correctly, in my comment, I was saying that DBSubnetGroupParameters should have a Tags field and it seems to be the case now.

pkg/controller/database/dbsubnetgroup/controller.go

muvaf · 2020-04-01T20:39:29Z

apis/database/v1beta1/dbsubnetgroup_types.go

+	SubnetGroupStatus string `json:"subnetGroupStatus,omitempty"`
+
+	// DBSubnetGroupArn is the Amazon Resource Name (ARN) for this DB subnet group.
+	DBSubnetGroupArn string `json:"dbSubnetGroupArn,omitempty"`


I think state and arn would be sufficient for names as it's already implied with these fields being under DBSubnetGroup CRD. I see that they are indeed named as DBSubnetGroupArn and SubnetGroupStatus in cloud API but IMO we should go with state and arn as it's really unnecessary.

muvaf · 2020-04-01T20:39:58Z

apis/database/v1beta1/dbsubnetgroup_types.go

-// +kubebuilder:printcolumn:name="STATUS",type="string",JSONPath=".status.groupStatus"
+// +kubebuilder:printcolumn:name="GROUPNAME",type="string",JSONPath=".spec.forProvider.groupName"
+// +kubebuilder:printcolumn:name="DESCRIPTION",type="string",JSONPath=".spec.forProvider.description"
+// +kubebuilder:printcolumn:name="STATUS",type="string",JSONPath=".status.atProvider.groupStatus"


Suggested change

// +kubebuilder:printcolumn:name="STATUS",type="string",JSONPath=".status.atProvider.groupStatus"

// +kubebuilder:printcolumn:name="STATUS",type="string",JSONPath=".status.atProvider.subnetGroupStatus"

muvaf · 2020-04-01T20:40:09Z

apis/database/v1beta1/dbsubnetgroup_types.go

-// +kubebuilder:printcolumn:name="GROUPNAME",type="string",JSONPath=".spec.groupName"
-// +kubebuilder:printcolumn:name="DESCRIPTION",type="string",JSONPath=".spec.description"
-// +kubebuilder:printcolumn:name="STATUS",type="string",JSONPath=".status.groupStatus"
+// +kubebuilder:printcolumn:name="GROUPNAME",type="string",JSONPath=".spec.forProvider.groupName"


Suggested change

// +kubebuilder:printcolumn:name="GROUPNAME",type="string",JSONPath=".spec.forProvider.groupName"

muvaf · 2020-04-01T20:41:48Z

pkg/clients/dbsubnetgroup/dbsubnetgroup.go

+
+// CreatePatch creates a *v1beta1.DBSubnetGroupParameters that has only the changed
+// values between the target *v1beta1.DBSubnetGroupParameters and the current *rds.DBSubnetGroup
+func CreatePatch(in *rds.DBSubnetGroup, target *v1beta1.DBSubnetGroupParameters) (*v1beta1.DBSubnetGroupParameters, error) {


I don't see any usage of func CreatePatch other than is-up-to-date check, so, it'd be better if you can remove it after making the is-up-to-date simpler.

pkg/controller/database/dbsubnetgroup/controller.go

muvaf · 2020-04-06T15:45:13Z

pkg/clients/dbsubnetgroup/dbsubnetgroup.go

+}
+
+// IsDBSubnetGroupUpToDate checks whether there is a change in any of the modifiable fields.
+func IsDBSubnetGroupUpToDate(p v1beta1.DBSubnetGroupParameters, sg rds.DBSubnetGroup) (bool, error) {


This comment assumed that we were not able to update the tags but that's actually possible later I realized. So, you need to check tags as well.

muvaf · 2020-04-06T15:52:28Z

apis/database/v1beta1/dbsubnetgroup_types.go

 	Tags []Tag `json:"tags,omitempty"`
 }

 // A DBSubnetGroupSpec defines the desired state of a DBSubnetGroup.
 type DBSubnetGroupSpec struct {
 	runtimev1alpha1.ResourceSpec `json:",inline"`
-	DBSubnetGroupParameters      `json:",inline"`
+	ForProvider                  DBSubnetGroupParameters `json:"forProvider,omitempty"`
 }

 // DBSubnetGroupExternalStatus keeps the state for the external resource


DBSubnetGroupExternalStatus doesn't seem to be used anywhere, so you might as well remove it.

apis/database/v1beta1/dbsubnetgroup_types.go

muvaf · 2020-04-06T15:57:04Z

apis/database/v1beta1/dbsubnetgroup_types.go

 		if id == value {
 			return nil
 		}
 	}

-	sg.Spec.SubnetIDs = append(sg.Spec.SubnetIDs, value)
+	sg.Spec.ForProvider.SubnetIDs = append(sg.Spec.ForProvider.SubnetIDs, value)
 	return nil
 }



I think you can remove the line var _ resource.AttributeReferencer = (*SubnetIDReferencerForDBSubnetGroup)(nil) as that struct already has its own test file to test this interface conformance.

pkg/controller/database/dbsubnetgroup/controller.go

muvaf · 2020-04-06T16:57:47Z

Please make sure the functionality works as expected by testing manually.

muvaf

There are a few nitpicks unaddressed but the PR should be ready to go. @gauravgahlot thank you!

pkg/controller/database/dbsubnetgroup/controller.go

hasheddan

A few nitpicks, but I think we can move forward and circle back around for any clean-up. Thanks for your great work @gauravgahlot! Could you post any manual testing you have done as a comment? (or point me to where you already have if I missed it scrolling through the comments)

hasheddan · 2020-04-07T22:39:28Z

apis/database/v1beta1/dbsubnetgroup_referencers_test.go

@@ -167,7 +166,8 @@ func TestDBSubnetGroupNameReferencerBuild(t *testing.T) {
 			input: input{
 				readerFn: func(ctx context.Context, key client.ObjectKey, obj runtime.Object) error {
 					p := obj.(*DBSubnetGroup)
-					p.Spec.DBSubnetGroupName = mockDBSubnetGroupName
+					// p.Spec.ForProvider.DBSubnetGroupName = mockDBSubnetGroupName


I think this can be removed here

hasheddan · 2020-04-07T22:42:42Z

apis/database/v1beta1/dbsubnetgroup_types.go

-			SubnetStatus: aws.StringValue(sn.SubnetStatus),
-		}
-	}
+// DBSubnetGroupObservation is the representation of the current state that is observed


nitpick: would you mind moving this up above DBSubnetGroupStatus where DBSubnetGroupExternalStatus is currently?

hasheddan · 2020-04-07T22:43:51Z

pkg/clients/dbsubnetgroup/dbsubnetgroup.go

+)
+
+// Client is the external client used for DBSubnetGroup Custom Resource
+type Client interface { // nolint:gocyclo


nitpick: is // nolint:gocyclo needed here?

hasheddan · 2020-04-07T22:46:59Z

pkg/clients/rds/fake/dbsubnetgroup.go

@@ -19,17 +19,20 @@ package fake
 import (


nitpick: might make sense to move this file fake/dbsubnetgroup.go from clients/rds to clients/dbsubnetgroup

hasheddan

LGTM!

muvaf

LGTM! You can consider squash some commits and/or clean up commit messages before the merge but that shouldn't block merging. cc @negz for merge.

Signed-off-by: Gaurav Gahlot <ggahlot@infracloud.io>

Signed-off-by: Gaurav Gahlot <gaurav.gahlot19@gmail.com>

Signed-off-by: Gaurav Gahlot <ggahlot@infracloud.io>

Signed-off-by: Gaurav Gahlot <gaurav.gahlot19@gmail.com>

Move DBSubnetGroup to v1beta1 Signed-off-by: Gaurav Gahlot <ggahlot@infracloud.io> Signed-off-by: ajaykangare <ajaykangare1993@gmail.com>

Move DBSubnetGroup to v1beta1 Signed-off-by: Gaurav Gahlot <ggahlot@infracloud.io>

Move DBSubnetGroup to v1beta1

gauravgahlot mentioned this pull request Mar 23, 2020

AWS DBSubnetGroup Doc updates for v1beta1 crossplane/crossplane#1360

Merged

3 tasks

gauravgahlot force-pushed the v1beta1-dbsubnetgroup branch from 2c1ff5a to c67027e Compare March 23, 2020 08:33

hasheddan requested changes Mar 25, 2020

View reviewed changes

pkg/controller/database/dbsubnetgroup/controller.go Show resolved Hide resolved

infinitecompute linked an issue Mar 25, 2020 that may be closed by this pull request

DBSubnet Group from Alpha to Beta #165

Closed

muvaf reviewed Mar 25, 2020

View reviewed changes

infinitecompute mentioned this pull request Mar 25, 2020

Additional AWS managed resources #149

Closed

26 tasks

infinitecompute added p1 new-resource labels Mar 26, 2020

prasek added this to the v0.8 milestone Mar 26, 2020

gauravgahlot self-assigned this Mar 27, 2020

gauravgahlot force-pushed the v1beta1-dbsubnetgroup branch from 88b0b8f to 885711a Compare March 30, 2020 21:49

negz mentioned this pull request Mar 30, 2020

Support backup and restore of all AWS managed resources #181

Closed

7 tasks

gauravgahlot requested a review from muvaf March 30, 2020 22:08

muvaf reviewed Apr 1, 2020

View reviewed changes

pkg/controller/database/dbsubnetgroup/controller.go Outdated Show resolved Hide resolved

gauravgahlot force-pushed the v1beta1-dbsubnetgroup branch from 1e84685 to a0539fc Compare April 3, 2020 16:40

gauravgahlot requested a review from muvaf April 3, 2020 16:47

muvaf reviewed Apr 6, 2020

View reviewed changes

muvaf requested changes Apr 6, 2020

View reviewed changes

gauravgahlot requested a review from muvaf April 7, 2020 11:32

muvaf approved these changes Apr 7, 2020

View reviewed changes

pkg/controller/database/dbsubnetgroup/controller.go Outdated Show resolved Hide resolved

muvaf requested a review from hasheddan April 7, 2020 22:31

hasheddan approved these changes Apr 7, 2020

View reviewed changes

gauravgahlot requested review from hasheddan and muvaf April 8, 2020 00:03

hasheddan approved these changes Apr 8, 2020

View reviewed changes

muvaf approved these changes Apr 8, 2020

View reviewed changes

Gaurav Gahlot and others added 2 commits April 8, 2020 17:19

dbsubnetgroup: moved DBSubnetGroup to v1beta1

f1ec5af

Signed-off-by: Gaurav Gahlot <ggahlot@infracloud.io>

dbsubnetgroup: updated controller and tests

478aaeb

Signed-off-by: Gaurav Gahlot <gaurav.gahlot19@gmail.com>

gauravgahlot and others added 6 commits April 8, 2020 17:19

dbsubnetgroup: review fixes

f17dd77

Signed-off-by: Gaurav Gahlot <gaurav.gahlot19@gmail.com>

dbsubnetgroup: v1alpha1-dbsubnetgroup: removed redundant files

99a9fc5

Signed-off-by: Gaurav Gahlot <ggahlot@infracloud.io>

dbsubnetgroup: DBSubnetGroup: modify

2546696

Signed-off-by: Gaurav Gahlot <ggahlot@infracloud.io>

dbsubnetgroup: the new CRD

6d0938e

Signed-off-by: Gaurav Gahlot <ggahlot@infracloud.io>

dbsubnetgroup: extracted client for DBSubnetGroup to a separate package

f81cbec

Signed-off-by: Gaurav Gahlot <gaurav.gahlot19@gmail.com>

dbsubnetgroup: using DBSubnetGroupObservation and ExternalName

64addbb

Signed-off-by: Gaurav Gahlot <gaurav.gahlot19@gmail.com>

gauravgahlot force-pushed the v1beta1-dbsubnetgroup branch from 64eb303 to f17dd77 Compare April 8, 2020 11:49

negz merged commit 7864298 into crossplane-contrib:master Apr 8, 2020

muvaf mentioned this pull request Apr 8, 2020

dbsubnetgroup: update to v1beta1 crossplane/stack-aws-sample#18

Merged

gauravgahlot deleted the v1beta1-dbsubnetgroup branch April 9, 2020 16:13

wolffbe pushed a commit to wolffbe/provider-aws that referenced this pull request Feb 12, 2021

Move DBSubnetGroup to v1beta1 (crossplane-contrib#162)

b1f3adb

Move DBSubnetGroup to v1beta1 Signed-off-by: Gaurav Gahlot <ggahlot@infracloud.io>

namku pushed a commit to namku/provider-aws that referenced this pull request Mar 9, 2021

Move DBSubnetGroup to v1beta1 (crossplane-contrib#162)

50efd5d

Move DBSubnetGroup to v1beta1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move DBSubnetGroup to v1beta1 #162

Move DBSubnetGroup to v1beta1 #162

gauravgahlot commented Mar 23, 2020 •

edited

Loading

muvaf Mar 25, 2020

muvaf Mar 25, 2020

muvaf left a comment

muvaf Apr 1, 2020

muvaf Apr 6, 2020

muvaf Apr 1, 2020

muvaf Apr 1, 2020

muvaf Apr 1, 2020

muvaf Apr 1, 2020

muvaf Apr 1, 2020

muvaf Apr 6, 2020

muvaf Apr 6, 2020

muvaf Apr 6, 2020

muvaf commented Apr 6, 2020

muvaf left a comment

hasheddan left a comment

hasheddan Apr 7, 2020

hasheddan Apr 7, 2020

hasheddan Apr 7, 2020

hasheddan Apr 7, 2020

hasheddan left a comment

muvaf left a comment

	DBSubnetGroupName: aws.String(cr.Spec.ForProvider.DBSubnetGroupName),
	DBSubnetGroupName: meta.GetExternalName(cr),

	_, err := req.Send(ctx)
	_, err := req.Send(ctx)
	return errors.Wrap(resource.Ignore(rds.IsDBSubnetGroupNotFoundErr, err), errDelete)

	// +kubebuilder:printcolumn:name="STATUS",type="string",JSONPath=".status.atProvider.groupStatus"
	// +kubebuilder:printcolumn:name="STATUS",type="string",JSONPath=".status.atProvider.subnetGroupStatus"

Move DBSubnetGroup to v1beta1 #162

Move DBSubnetGroup to v1beta1 #162

Conversation

gauravgahlot commented Mar 23, 2020 • edited Loading

Description of your changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muvaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muvaf commented Apr 6, 2020

muvaf left a comment

Choose a reason for hiding this comment

hasheddan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasheddan left a comment

Choose a reason for hiding this comment

muvaf left a comment

Choose a reason for hiding this comment

gauravgahlot commented Mar 23, 2020 •

edited

Loading