Repository: crossplane-contrib/provider-gitlab
Pull request #101 (2023-08-10 10:36:26)

fix(projects.member): Remove use of 'externalName' to store 'ProjectID' field value
<!--
Thank you for helping to improve Crossplane!

Please read through https://git.io/fj2m9 if this is your first time opening a
Crossplane pull request. Find us in https://slack.crossplane.io/messages/dev if
you need any help contributing.
-->

### Description of your changes

- Remove use of 'externalName' to store 'ProjectID' field value

<!--
Briefly describe what this pull request does. Be sure to direct your reviewers'
attention to anything that needs special consideration.

We love pull requests that resolve an open Crossplane issue. If yours does, you
can uncomment the below line to indicate which issue your PR fixes, for example
"Fixes #500":

-->

I have:

- [x] Read and followed Crossplane's [contribution process].
- [x] Run `make reviewable test` to ensure this PR is ready for review.

### How has this code been tested

<!--
Before reviewers can be confident in the correctness of this pull request, it
needs to tested and shown to be correct. Briefly describe the testing that has
already been done or which is planned for this change.
-->

[contribution process]: https://git.io/fj2m9



Patch:
diff --git a/pkg/controller/projects/members/controller.go b/pkg/controller/projects/members/controller.go
index 36479aa..1375980 100644
--- a/pkg/controller/projects/members/controller.go
+++ b/pkg/controller/projects/members/controller.go
@@ -15,7 +15,6 @@ package members
 
 import (
 	"context"
-	"strconv"
 
 	"github.com/xanzy/go-gitlab"
 
@@ -27,7 +26,6 @@ import (
 
 	"github.com/crossplane/crossplane-runtime/pkg/event"
 	"github.com/crossplane/crossplane-runtime/pkg/logging"
-	"github.com/crossplane/crossplane-runtime/pkg/meta"
 	"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"
 	"github.com/crossplane/crossplane-runtime/pkg/resource"
 
@@ -38,11 +36,11 @@ import (
 
 const (
 	errNotMember        = "managed resource is not a Gitlab Project Member custom resource"
-	errProjectIDMissing = "Project ID missing"
 	errCreateFailed     = "cannot create Gitlab Project Member"
 	errUpdateFailed     = "cannot update Gitlab Project Member"
 	errDeleteFailed     = "cannot delete Gitlab Project Member"
 	errObserveFailed    = "cannot observe Gitlab Project Member"
+	errProjectIDMissing = "ProjectID is missing"
 )
 
 // SetupMember adds a controller that reconciles Project Members.
@@ -87,18 +85,15 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
 	if !ok {
 		return managed.ExternalObservation{}, errors.New(errNotMember)
 	}
-
-	externalName := meta.GetExternalName(cr)
-	if externalName == "" {
-		return managed.ExternalObservation{ResourceExists: false}, nil
+	if cr.Spec.ForProvider.ProjectID == nil {
+		return managed.ExternalObservation{}, errors.New(errProjectIDMissing)
 	}
 
-	projectMemberID, err := strconv.Atoi(externalName)
-	if err != nil {
-		return managed.ExternalObservation{}, errors.New(errNotMember)
-	}
+	projectMember, res, err := e.client.GetProjectMember(
+		*cr.Spec.ForProvider.ProjectID,
+		cr.Spec.ForProvider.UserID,
+	)
 
-	projectMember, res, err := e.client.GetProjectMember(projectMemberID, cr.Spec.ForProvider.UserID)
 	if err != nil {
 		if clients.IsResponseNotFound(res) {
 			return managed.ExternalObservation{}, nil
@@ -134,7 +129,6 @@ func (e *external) Create(ctx context.Context, mg resource.Managed) (managed.Ext
 		return managed.ExternalCreation{}, errors.Wrap(err, errCreateFailed)
 	}
 
-	meta.SetExternalName(cr, strconv.Itoa(*cr.Spec.ForProvider.ProjectID))
 	return managed.ExternalCreation{ExternalNameAssigned: true}, nil
 }
 
@@ -143,8 +137,12 @@ func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext
 	if !ok {
 		return managed.ExternalUpdate{}, errors.New(errNotMember)
 	}
+	if cr.Spec.ForProvider.ProjectID == nil {
+		return managed.ExternalUpdate{}, errors.New(errProjectIDMissing)
+	}
+
 	_, _, err := e.client.EditProjectMember(
-		meta.GetExternalName(cr),
+		*cr.Spec.ForProvider.ProjectID,
 		cr.Spec.ForProvider.UserID,
 		projects.GenerateEditMemberOptions(&cr.Spec.ForProvider),
 		gitlab.WithContext(ctx),
@@ -157,9 +155,12 @@ func (e *external) Delete(ctx context.Context, mg resource.Managed) error {
 	if !ok {
 		return errors.New(errNotMember)
 	}
+	if cr.Spec.ForProvider.ProjectID == nil {
+		return errors.New(errProjectIDMissing)
+	}
 
 	_, err := e.client.DeleteProjectMember(
-		meta.GetExternalName(cr),
+		*cr.Spec.ForProvider.ProjectID,
 		cr.Spec.ForProvider.UserID,
 		gitlab.WithContext(ctx),
 	)
@@ -168,7 +169,6 @@ func (e *external) Delete(ctx context.Context, mg resource.Managed) error {
 
 // isMemberUpToDate checks whether there is a change in any of the modifiable fields.
 func isMemberUpToDate(p *v1alpha1.MemberParameters, g *gitlab.ProjectMember) bool { // nolint:gocyclo
-
 	if !cmp.Equal(int(p.AccessLevel), int(g.AccessLevel)) {
 		return false
 	}


Test patch:
diff --git a/pkg/controller/projects/members/controller_test.go b/pkg/controller/projects/members/controller_test.go
index 09c7a8e..9a8f41b 100644
--- a/pkg/controller/projects/members/controller_test.go
+++ b/pkg/controller/projects/members/controller_test.go
@@ -16,7 +16,6 @@ package members
 import (
 	"context"
 	"net/http"
-	"strconv"
 	"testing"
 	"time"
 
@@ -26,7 +25,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
-	"github.com/crossplane/crossplane-runtime/pkg/meta"
 	"github.com/crossplane/crossplane-runtime/pkg/reconciler/managed"
 	"github.com/crossplane/crossplane-runtime/pkg/resource"
 	"github.com/crossplane/crossplane-runtime/pkg/test"
@@ -37,21 +35,20 @@ import (
 )
 
 var (
-	unexpecedItem     resource.Managed
-	errBoom           = errors.New("boom")
-	ID                = 0
-	extName           = strconv.Itoa(ID)
-	extNameAnnotation = map[string]string{meta.AnnotationKeyExternalName: extName}
-	username          = "username"
-	name              = "name"
-	state             = "state"
-	avatarURL         = "http://avatarURL"
-	webURL            = "http://webURL"
-	email             = "email@gmail.com"
-	accessLevel       = gitlab.AccessLevelValue(30)
-	now               = time.Now()
-	expiresAt         = gitlab.ISOTime(now.AddDate(0, 0, 7*3))
-	expiresAtNew      = gitlab.ISOTime(now.AddDate(0, 0, 7*4))
+	unexpecedItem resource.Managed
+	errBoom       = errors.New("boom")
+	ID            = 0
+	username      = "username"
+	name          = "name"
+	state         = "state"
+	avatarURL     = "http://avatarURL"
+	webURL        = "http://webURL"
+	email         = "email@gmail.com"
+	accessLevel   = gitlab.AccessLevelValue(30)
+	now           = time.Now()
+	expiresAt     = gitlab.ISOTime(now.AddDate(0, 0, 7*3))
+	expiresAtNew  = gitlab.ISOTime(now.AddDate(0, 0, 7*4))
+	projectID     = 1234
 )
 
 type args struct {
@@ -74,8 +71,8 @@ func withConditions(c ...xpv1.Condition) projectModifier {
 	return func(cr *v1alpha1.Member) { cr.Status.ConditionedStatus.Conditions = c }
 }
 
-func withExternalName(n string) projectModifier {
-	return func(r *v1alpha1.Member) { meta.SetExternalName(r, n) }
+func withProjectID() projectModifier {
+	return func(r *v1alpha1.Member) { r.Spec.ForProvider.ProjectID = &projectID }
 }
 
 func withStatus(s v1alpha1.MemberObservation) projectModifier {
@@ -86,10 +83,6 @@ func withSpec(s v1alpha1.MemberParameters) projectModifier {
 	return func(r *v1alpha1.Member) { r.Spec.ForProvider = s }
 }
 
-func withAnnotations(a map[string]string) projectModifier {
-	return func(p *v1alpha1.Member) { meta.AddAnnotations(p, a) }
-}
-
 func projectMember(m ...projectModifier) *v1alpha1.Member {
 	cr := &v1alpha1.Member{}
 	for _, f := range m {
@@ -166,7 +159,7 @@ func TestObserve(t *testing.T) {
 				err: errors.New(errNotMember),
 			},
 		},
-		"NoExternalName": {
+		"ErrProjectIDMissing": {
 			args: args{
 				cr: projectMember(),
 			},
@@ -177,20 +170,7 @@ func TestObserve(t *testing.T) {
 					ResourceUpToDate:        false,
 					ResourceLateInitialized: false,
 				},
-			},
-		},
-		"NotIDExternalName": {
-			args: args{
-				projectMember: &fake.MockClient{
-					MockGetMember: func(pid interface{}, user int, options ...gitlab.RequestOptionFunc) (*gitlab.ProjectMember, *gitlab.Response, error) {
-						return &gitlab.ProjectMember{}, &gitlab.Response{}, nil
-					},
-				},
-				cr: projectMember(withExternalName("fr")),
-			},
-			want: want{
-				cr:  projectMember(withExternalName("fr")),
-				err: errors.New(errNotMember),
+				err: errors.New(errProjectIDMissing),
 			},
 		},
 		"ErrGet404": {
@@ -200,10 +180,10 @@ func TestObserve(t *testing.T) {
 						return nil, &gitlab.Response{Response: &http.Response{StatusCode: 404}}, errBoom
 					},
 				},
-				cr: projectMember(withExternalName(extName)),
+				cr: projectMember(withProjectID()),
 			},
 			want: want{
-				cr:     projectMember(withAnnotations(extNameAnnotation)),
+				cr:     projectMember(withProjectID()),
 				result: managed.ExternalObservation{ResourceExists: false},
 				err:    nil,
 			},
@@ -215,10 +195,10 @@ func TestObserve(t *testing.T) {
 						return nil, nil, errBoom
 					},
 				},
-				cr: projectMember(withExternalName(extName)),
+				cr: projectMember(withProjectID()),
 			},
 			want: want{
-				cr:     projectMember(withAnnotations(extNameAnnotation)),
+				cr:     projectMember(withProjectID()),
 				result: managed.ExternalObservation{ResourceExists: false},
 				err:    errors.Wrap(errBoom, errObserveFailed),
 			},
@@ -231,14 +211,14 @@ func TestObserve(t *testing.T) {
 					},
 				},
 				cr: projectMember(
-					withExternalName(extName),
+					withProjectID(),
 				),
 			},
 			want: want{
 				cr: projectMember(
 					withConditions(xpv1.Available()),
-					withAnnotations(extNameAnnotation),
-					withExternalName(extName),
+					withProjectID(),
+					withProjectID(),
 				),
 				result: managed.ExternalObservation{
 					ResourceExists:          true,
@@ -257,15 +237,15 @@ func TestObserve(t *testing.T) {
 					},
 				},
 				cr: projectMember(
-					withExternalName(extName),
+					withProjectID(),
 					withAccessLevel(10),
 				),
 			},
 			want: want{
 				cr: projectMember(
 					withConditions(xpv1.Available()),
-					withAnnotations(extNameAnnotation),
-					withExternalName(extName),
+					withProjectID(),
+					withProjectID(),
 					withAccessLevel(10),
 				),
 				result: managed.ExternalObservation{
@@ -285,15 +265,15 @@ func TestObserve(t *testing.T) {
 					},
 				},
 				cr: projectMember(
-					withExternalName(extName),
+					withProjectID(),
 					withExpiresAt(expiresAtNew.String()),
 				),
 			},
 			want: want{
 				cr: projectMember(
 					withConditions(xpv1.Available()),
-					withAnnotations(extNameAnnotation),
-					withExternalName(extName),
+					withProjectID(),
+					withProjectID(),
 					withExpiresAt(expiresAtNew.String()),
 				),
 				result: managed.ExternalObservation{
@@ -363,13 +343,13 @@ func TestCreate(t *testing.T) {
 					},
 				},
 				cr: projectMember(
-					withAnnotations(extNameAnnotation),
+					withProjectID(),
 					withSpec(v1alpha1.MemberParameters{ProjectID: &ID}),
 				),
 			},
 			want: want{
 				cr: projectMember(
-					withExternalName(extName),
+					withProjectID(),
 					withSpec(v1alpha1.MemberParameters{ProjectID: &ID}),
 				),
 				result: managed.ExternalCreation{ExternalNameAssigned: true},
@@ -396,13 +376,13 @@ func TestCreate(t *testing.T) {
 					},
 				},
 				cr: projectMember(
-					withAnnotations(extNameAnnotation),
+					withProjectID(),
 					withSpec(v1alpha1.MemberParameters{ProjectID: &ID}),
 				),
 			},
 			want: want{
 				cr: projectMember(
-					withExternalName(extName),
+					withProjectID(),
 					withSpec(v1alpha1.MemberParameters{ProjectID: &ID}),
 				),
 				result: managed.ExternalCreation{ExternalNameAssigned: true},
@@ -416,13 +396,13 @@ func TestCreate(t *testing.T) {
 					},
 				},
 				cr: projectMember(
-					withAnnotations(extNameAnnotation),
+					withProjectID(),
 					withSpec(v1alpha1.MemberParameters{ProjectID: &ID}),
 				),
 			},
 			want: want{
 				cr: projectMember(
-					withExternalName(extName),
+					withProjectID(),
 					withSpec(v1alpha1.MemberParameters{ProjectID: &ID}),
 				),
 				err: errors.Wrap(errBoom, errCreateFailed),
@@ -485,13 +465,13 @@ func TestUpdate(t *testing.T) {
 					},
 				},
 				cr: projectMember(
-					withExternalName(extName),
+					withProjectID(),
 					withStatus(v1alpha1.MemberObservation{Username: "new username"}),
 				),
 			},
 			want: want{
 				cr: projectMember(
-					withExternalName(extName),
+					withProjectID(),
 					withStatus(v1alpha1.MemberObservation{Username: "new username"}),
 				),
 			},
@@ -503,10 +483,10 @@ func TestUpdate(t *testing.T) {
 						return &gitlab.ProjectMember{}, &gitlab.Response{}, errBoom
 					},
 				},
-				cr: projectMember(),
+				cr: projectMember(withProjectID()),
 			},
 			want: want{
-				cr:  projectMember(),
+				cr:  projectMember(withProjectID()),
 				err: errors.Wrap(errBoom, errUpdateFailed),
 			},
 		},
@@ -555,10 +535,10 @@ func TestDelete(t *testing.T) {
 						return &gitlab.Response{}, nil
 					},
 				},
-				cr: projectMember(withExternalName(extName)),
+				cr: projectMember(withProjectID()),
 			},
 			want: want{
-				cr:  projectMember(withExternalName(extName)),
+				cr:  projectMember(withProjectID()),
 				err: nil,
 			},
 		},
@@ -569,10 +549,10 @@ func TestDelete(t *testing.T) {
 						return &gitlab.Response{}, errBoom
 					},
 				},
-				cr: projectMember(),
+				cr: projectMember(withProjectID()),
 			},
 			want: want{
-				cr:  projectMember(),
+				cr:  projectMember(withProjectID()),
 				err: errors.Wrap(errBoom, errDeleteFailed),
 			},
 		},