Skip to content

Commit

Permalink
fix(groups.member): Remove setting of 'externalName' as GroupID
Browse files Browse the repository at this point in the history 
Signed-off-by: Vitalijs <[email protected]>
  • Loading branch information
Vitalijs committed Aug 10, 2023
1 parent e69095f commit 8cf71b4
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 93 deletions.
31 changes: 14 additions & 17 deletions pkg/controller/groups/members/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ package members

import (
"context"
"strconv"

"github.com/xanzy/go-gitlab"

Expand All @@ -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"

Expand Down Expand Up @@ -89,17 +87,13 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex
return managed.ExternalObservation{}, errors.New(errNotMember)
}

externalName := meta.GetExternalName(cr)
if externalName == "" {
return managed.ExternalObservation{ResourceExists: false}, nil
}

groupID, err := strconv.Atoi(externalName)
if err != nil {
return managed.ExternalObservation{}, errors.New(errIDNotInt)
if cr.Spec.ForProvider.GroupID == nil {
return managed.ExternalObservation{}, errors.New(errMissingGroupID)
}

groupMember, res, err := e.client.GetGroupMember(groupID, cr.Spec.ForProvider.UserID)
groupMember, res, err := e.client.GetGroupMember(
*cr.Spec.ForProvider.GroupID,
cr.Spec.ForProvider.UserID,
)
if err != nil {
if clients.IsResponseNotFound(res) {
return managed.ExternalObservation{}, nil
Expand Down Expand Up @@ -136,7 +130,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.GroupID))
return managed.ExternalCreation{ExternalNameAssigned: true}, nil
}

Expand All @@ -145,9 +138,11 @@ func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext
if !ok {
return managed.ExternalUpdate{}, errors.New(errNotMember)
}

if cr.Spec.ForProvider.GroupID == nil {
return managed.ExternalUpdate{}, errors.New(errMissingGroupID)
}
_, _, err := e.client.EditGroupMember(
meta.GetExternalName(cr),
*cr.Spec.ForProvider.GroupID,
cr.Spec.ForProvider.UserID,
groups.GenerateEditMemberOptions(&cr.Spec.ForProvider),
gitlab.WithContext(ctx),
Expand All @@ -160,9 +155,11 @@ func (e *external) Delete(ctx context.Context, mg resource.Managed) error {
if !ok {
return errors.New(errNotMember)
}

if cr.Spec.ForProvider.GroupID == nil {
return errors.New(errMissingGroupID)
}
_, err := e.client.RemoveGroupMember(
meta.GetExternalName(cr),
*cr.Spec.ForProvider.GroupID,
cr.Spec.ForProvider.UserID,
nil,
gitlab.WithContext(ctx),
Expand Down
112 changes: 36 additions & 76 deletions pkg/controller/groups/members/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package members
import (
"context"
"net/http"
"strconv"
"testing"
"time"

Expand All @@ -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"
Expand All @@ -37,20 +35,19 @@ 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"
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"
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))
groupID = 1234
)

type args struct {
Expand All @@ -73,8 +70,8 @@ func withConditions(c ...xpv1.Condition) groupModifier {
return func(cr *v1alpha1.Member) { cr.Status.ConditionedStatus.Conditions = c }
}

func withExternalName(n string) groupModifier {
return func(r *v1alpha1.Member) { meta.SetExternalName(r, n) }
func withGroupID() groupModifier {
return func(r *v1alpha1.Member) { r.Spec.ForProvider.GroupID = &groupID }
}

func withStatus(s v1alpha1.MemberObservation) groupModifier {
Expand All @@ -85,10 +82,6 @@ func withSpec(s v1alpha1.MemberParameters) groupModifier {
return func(r *v1alpha1.Member) { r.Spec.ForProvider = s }
}

func withAnnotations(a map[string]string) groupModifier {
return func(p *v1alpha1.Member) { meta.AddAnnotations(p, a) }
}

func groupMember(m ...groupModifier) *v1alpha1.Member {
cr := &v1alpha1.Member{}
for _, f := range m {
Expand Down Expand Up @@ -165,44 +158,17 @@ func TestObserve(t *testing.T) {
err: errors.New(errNotMember),
},
},
"NoExternalName": {
args: args{
cr: groupMember(),
},
want: want{
cr: groupMember(),
result: managed.ExternalObservation{
ResourceExists: false,
ResourceUpToDate: false,
ResourceLateInitialized: false,
},
},
},
"NotIDExternalName": {
args: args{
groupMember: &fake.MockClient{
MockGetMember: func(gid interface{}, user int, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) {
return &gitlab.GroupMember{}, &gitlab.Response{}, nil
},
},
cr: groupMember(withExternalName("fr")),
},
want: want{
cr: groupMember(withExternalName("fr")),
err: errors.New(errIDNotInt),
},
},
"FailedGetRequest": {
args: args{
groupMember: &fake.MockClient{
MockGetMember: func(gid interface{}, user int, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) {
return nil, &gitlab.Response{Response: &http.Response{StatusCode: 400}}, errBoom
},
},
cr: groupMember(withExternalName(extName)),
cr: groupMember(withGroupID()),
},
want: want{
cr: groupMember(withAnnotations(extNameAnnotation)),
cr: groupMember(withGroupID()),
result: managed.ExternalObservation{ResourceExists: false},
err: errors.Wrap(errBoom, errGetFailed),
},
Expand All @@ -214,10 +180,10 @@ func TestObserve(t *testing.T) {
return nil, &gitlab.Response{Response: &http.Response{StatusCode: 404}}, errBoom
},
},
cr: groupMember(withExternalName(extName)),
cr: groupMember(withGroupID()),
},
want: want{
cr: groupMember(withAnnotations(extNameAnnotation)),
cr: groupMember(withGroupID()),
result: managed.ExternalObservation{ResourceExists: false},
err: nil,
},
Expand All @@ -230,14 +196,13 @@ func TestObserve(t *testing.T) {
},
},
cr: groupMember(
withExternalName(extName),
withGroupID(),
),
},
want: want{
cr: groupMember(
withConditions(xpv1.Available()),
withAnnotations(extNameAnnotation),
withExternalName(extName),
withGroupID(),
),
result: managed.ExternalObservation{
ResourceExists: true,
Expand All @@ -256,15 +221,14 @@ func TestObserve(t *testing.T) {
},
},
cr: groupMember(
withExternalName(extName),
withGroupID(),
withAccessLevel(10),
),
},
want: want{
cr: groupMember(
withConditions(xpv1.Available()),
withAnnotations(extNameAnnotation),
withExternalName(extName),
withGroupID(),
withAccessLevel(10),
),
result: managed.ExternalObservation{
Expand All @@ -284,15 +248,14 @@ func TestObserve(t *testing.T) {
},
},
cr: groupMember(
withExternalName(extName),
withGroupID(),
withExpiresAt(expiresAtNew.String()),
),
},
want: want{
cr: groupMember(
withConditions(xpv1.Available()),
withAnnotations(extNameAnnotation),
withExternalName(extName),
withGroupID(),
withExpiresAt(expiresAtNew.String()),
),
result: managed.ExternalObservation{
Expand Down Expand Up @@ -361,13 +324,12 @@ func TestCreate(t *testing.T) {
},
},
cr: groupMember(
withAnnotations(extNameAnnotation),
withSpec(v1alpha1.MemberParameters{GroupID: &ID}),
),
},
want: want{
cr: groupMember(
withExternalName(extName),
withGroupID(),
withSpec(v1alpha1.MemberParameters{GroupID: &ID}),
),
result: managed.ExternalCreation{ExternalNameAssigned: true},
Expand All @@ -393,13 +355,12 @@ func TestCreate(t *testing.T) {
},
},
cr: groupMember(
withAnnotations(extNameAnnotation),
withSpec(v1alpha1.MemberParameters{GroupID: &ID}),
),
},
want: want{
cr: groupMember(
withExternalName(extName),
withGroupID(),
withSpec(v1alpha1.MemberParameters{GroupID: &ID}),
),
result: managed.ExternalCreation{ExternalNameAssigned: true},
Expand All @@ -413,13 +374,12 @@ func TestCreate(t *testing.T) {
},
},
cr: groupMember(
withAnnotations(extNameAnnotation),
withSpec(v1alpha1.MemberParameters{GroupID: &ID}),
),
},
want: want{
cr: groupMember(
withExternalName(extName),
withGroupID(),
withSpec(v1alpha1.MemberParameters{GroupID: &ID}),
),
err: errors.Wrap(errBoom, errCreateFailed),
Expand Down Expand Up @@ -481,13 +441,13 @@ func TestUpdate(t *testing.T) {
},
},
cr: groupMember(
withExternalName(extName),
withGroupID(),
withStatus(v1alpha1.MemberObservation{Username: "new username"}),
),
},
want: want{
cr: groupMember(
withExternalName(extName),
withGroupID(),
withStatus(v1alpha1.MemberObservation{Username: "new username"}),
),
},
Expand All @@ -499,10 +459,10 @@ func TestUpdate(t *testing.T) {
return &gitlab.GroupMember{}, &gitlab.Response{}, errBoom
},
},
cr: groupMember(),
cr: groupMember(withGroupID()),
},
want: want{
cr: groupMember(),
cr: groupMember(withGroupID()),
err: errors.Wrap(errBoom, errUpdateFailed),
},
},
Expand Down Expand Up @@ -551,10 +511,10 @@ func TestDelete(t *testing.T) {
return &gitlab.Response{}, nil
},
},
cr: groupMember(withExternalName(extName)),
cr: groupMember(withGroupID()),
},
want: want{
cr: groupMember(withExternalName(extName)),
cr: groupMember(withGroupID()),
err: nil,
},
},
Expand All @@ -565,10 +525,10 @@ func TestDelete(t *testing.T) {
return &gitlab.Response{}, errBoom
},
},
cr: groupMember(),
cr: groupMember(withGroupID()),
},
want: want{
cr: groupMember(),
cr: groupMember(withGroupID()),
err: errors.Wrap(errBoom, errDeleteFailed),
},
},
Expand Down

0 comments on commit 8cf71b4

Please sign in to comment.