Repository: crossplane-contrib/provider-gitlab
Pull request #168 (2024-11-24 15:26:29)

feat: Add immediate deletion of groups
### Description of your changes

On GitLab [Premium](https://about.gitlab.com/pricing/premium/) and [Ultimate](https://about.gitlab.com/pricing/ultimate/), the delete action on a group just marks the group as deleted. By default, the group will be deleted seven days in the future.  To delete the group immediately two rest calls are needed:

1. DELETE request to `/groups/:id` which marks the group in the "pending for deletion" state ([docs](https://docs.gitlab.com/ee/api/groups.html#delete-a-group))
2. DELETE request to `/groups/:id` with attributes `permanently_remove=true` and `full_path` set. The group will be deleted immediate.

This change introduce the optional fields `PermanentlyRemove` and `FullPath` on the GitLab group resource. This allows the user to enforce an immediate deletion of GitLab groups.

The same behavior exist by deleting projects (see #121, #149) and could be implemented in the same way as done here for groups. Before this feature can be implemented for projects, one needs to extend the _xanzy/go-gitlab_ client with `DeleteProjectOptions` along the lines of [DeleteGroupOptions](https://github.com/xanzy/go-gitlab/blob/main/groups.go#L617-L623).

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

Provider has been built locally and deployed on a AKS cluster. Deletion of GitLab Groups has been successfully end to end tested with and without PermanentlyRemove and FullPath fields set.

[contribution process]: https://git.io/fj2m9


dariozachow: can you rebase on main?
buehlmann: @dariozachow MR rebased on master
dariozachow: Unfortunately this only works for subgroups. Is there some way we can filter for this error ``permanently_remove` option is only available for subgroups` after trying the full deletion once? Or ignore top level groups at all
buehlmann: I would just skip the newly introduced deletion statement if the group to delete is on the root level. Determine if it's a root group, I would by testing `group.path == group.full_path`. If this condition is true, the group is on the root level. What do you think?
dariozachow: sounds good to me
buehlmann: @dariozachow delete logic adjusted as discussed

Patch:
diff --git a/apis/groups/v1alpha1/group_types.go b/apis/groups/v1alpha1/group_types.go
index 21b8dfc..b5c6265 100644
--- a/apis/groups/v1alpha1/group_types.go
+++ b/apis/groups/v1alpha1/group_types.go
@@ -158,6 +158,16 @@ type GroupParameters struct {
 	// SharedWithGroups create links for sharing a group with another group.
 	// +optional
 	SharedWithGroups []SharedWithGroups `json:"sharedWithGroups,omitempty"`
+
+	// Force the immediate deletion of the group when removed. In GitLab Premium and Ultimate a group is by default
+	// just marked for deletion and removed permanently after seven days. Defaults to false.
+	// +optional
+	PermanentlyRemove *bool `json:"permanentlyRemove,omitempty"`
+
+	// Full path of group to delete permanently. Only required if PermanentlyRemove is set to true.
+	// GitLab Premium and Ultimate only.
+	// +optional
+	FullPathToRemove *string `json:"fullPathToRemove,omitempty"`
 }
 
 // AccessLevelValue represents a permission level within GitLab.
diff --git a/apis/groups/v1alpha1/zz_generated.deepcopy.go b/apis/groups/v1alpha1/zz_generated.deepcopy.go
index 56a3320..7da1d18 100644
--- a/apis/groups/v1alpha1/zz_generated.deepcopy.go
+++ b/apis/groups/v1alpha1/zz_generated.deepcopy.go
@@ -598,6 +598,16 @@ func (in *GroupParameters) DeepCopyInto(out *GroupParameters) {
 			(*in)[i].DeepCopyInto(&(*out)[i])
 		}
 	}
+	if in.PermanentlyRemove != nil {
+		in, out := &in.PermanentlyRemove, &out.PermanentlyRemove
+		*out = new(bool)
+		**out = **in
+	}
+	if in.FullPathToRemove != nil {
+		in, out := &in.FullPathToRemove, &out.FullPathToRemove
+		*out = new(string)
+		**out = **in
+	}
 }
 
 // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GroupParameters.
diff --git a/build b/build
index cc14f9c..6f2b7c5 160000
--- a/build
+++ b/build
@@ -1,1 +1,1 @@
-Subproject commit cc14f9cdac034e0eaaeb43479f57ee85d5490473
+Subproject commit 6f2b7c5f0a88eac426439d6d9b40abd919d89fbe
diff --git a/package/crds/groups.gitlab.crossplane.io_groups.yaml b/package/crds/groups.gitlab.crossplane.io_groups.yaml
index 613af1f..9ab7a73 100644
--- a/package/crds/groups.gitlab.crossplane.io_groups.yaml
+++ b/package/crds/groups.gitlab.crossplane.io_groups.yaml
@@ -95,6 +95,11 @@ spec:
                     description: Extra pipeline minutes quota for this group (purchased
                       in addition to the minutes included in the plan).
                     type: integer
+                  fullPathToRemove:
+                    description: |-
+                      Full path of group to delete permanently. Only required if PermanentlyRemove is set to true.
+                      GitLab Premium and Ultimate only.
+                    type: string
                   lfsEnabled:
                     description: Enable/disable Large File Storage (LFS) for the projects
                       in this group.
@@ -194,6 +199,11 @@ spec:
                   path:
                     description: The path of the group.
                     type: string
+                  permanentlyRemove:
+                    description: |-
+                      Force the immediate deletion of the group when removed. In GitLab Premium and Ultimate a group is by default
+                      just marked for deletion and removed permanently after seven days. Defaults to false.
+                    type: boolean
                   projectCreationLevel:
                     description: |-
                       developers can create projects in the group.
diff --git a/pkg/controller/groups/groups/group.go b/pkg/controller/groups/groups/group.go
index 1ac1de0..922bdfb 100644
--- a/pkg/controller/groups/groups/group.go
+++ b/pkg/controller/groups/groups/group.go
@@ -19,6 +19,7 @@ package groups
 import (
 	"context"
 	"strconv"
+	"strings"
 	"time"
 
 	xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
@@ -245,6 +246,19 @@ func (e *external) Delete(ctx context.Context, mg resource.Managed) (managed.Ext
 	}
 
 	_, err := e.client.DeleteGroup(meta.GetExternalName(cr), &gitlab.DeleteGroupOptions{}, gitlab.WithContext(ctx))
+	// if the group is for some reason already marked for deletion, we ignore the error and continue to delete the group permanently
+	if err != nil && !strings.Contains(err.Error(), "Group has been already marked for deletion") {
+		return managed.ExternalDelete{}, errors.Wrap(err, errDeleteFailed)
+	}
+
+	// permanent deletion is only available on subgroups; when executed against top-level groups the backend will return an error
+	isSubGroup := cr.Status.AtProvider.FullPath != nil && *cr.Status.AtProvider.FullPath != cr.Spec.ForProvider.Path
+	if cr.Spec.ForProvider.PermanentlyRemove != nil && *cr.Spec.ForProvider.PermanentlyRemove && isSubGroup {
+		_, err = e.client.DeleteGroup(meta.GetExternalName(cr), &gitlab.DeleteGroupOptions{
+			PermanentlyRemove: cr.Spec.ForProvider.PermanentlyRemove,
+			FullPath:          cr.Spec.ForProvider.FullPathToRemove,
+		}, gitlab.WithContext(ctx))
+	}
 	return managed.ExternalDelete{}, errors.Wrap(err, errDeleteFailed)
 }
 


Test patch:
diff --git a/pkg/controller/groups/groups/group_test.go b/pkg/controller/groups/groups/group_test.go
index cd13bd3..768e8c5 100644
--- a/pkg/controller/groups/groups/group_test.go
+++ b/pkg/controller/groups/groups/group_test.go
@@ -82,6 +82,14 @@ func withPath(s string) groupModifier {
 	return func(r *v1alpha1.Group) { r.Spec.ForProvider.Path = s }
 }
 
+func withPermanentlyRemove(b *bool) groupModifier {
+	return func(r *v1alpha1.Group) { r.Spec.ForProvider.PermanentlyRemove = b }
+}
+
+func withFullPathToRemove(s *string) groupModifier {
+	return func(r *v1alpha1.Group) { r.Spec.ForProvider.FullPathToRemove = s }
+}
+
 func withDescription(s *string) groupModifier {
 	return func(r *v1alpha1.Group) { r.Spec.ForProvider.Description = s }
 }
@@ -844,9 +852,16 @@ func TestUpdate(t *testing.T) {
 }
 
 func TestDelete(t *testing.T) {
+	type deleteGroupCalls struct {
+		Pid interface{}
+		Opt *gitlab.DeleteGroupOptions
+	}
+	var recordedCalls []deleteGroupCalls
+
 	type want struct {
-		cr  resource.Managed
-		err error
+		cr    resource.Managed
+		calls []deleteGroupCalls
+		err   error
 	}
 
 	cases := map[string]struct {
@@ -866,21 +881,23 @@ func TestDelete(t *testing.T) {
 			args: args{
 				group: &fake.MockClient{
 					MockDeleteGroup: func(pid interface{}, opt *gitlab.DeleteGroupOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Response, error) {
+						recordedCalls = append(recordedCalls, deleteGroupCalls{Pid: pid, Opt: opt})
 						return &gitlab.Response{}, nil
 					},
 				},
 				cr: group(withExternalName("0")),
 			},
 			want: want{
-				cr:  group(withExternalName("0")),
-				err: nil,
+				cr:    group(withExternalName("0")),
+				calls: []deleteGroupCalls{{Pid: "0", Opt: &gitlab.DeleteGroupOptions{}}},
+				err:   nil,
 			},
 		},
 		"FailedDeletion": {
 			args: args{
 				group: &fake.MockClient{
 					MockDeleteGroup: func(pid interface{}, opt *gitlab.DeleteGroupOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Response, error) {
-						return &gitlab.Response{}, errBoom
+						return nil, errBoom
 					},
 				},
 				cr: group(),
@@ -890,9 +907,66 @@ func TestDelete(t *testing.T) {
 				err: errors.Wrap(errBoom, errDeleteFailed),
 			},
 		},
+		"SuccessfulPermanentlyDeletion": {
+			args: args{
+				group: &fake.MockClient{
+					MockDeleteGroup: func(pid interface{}, opt *gitlab.DeleteGroupOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Response, error) {
+						recordedCalls = append(recordedCalls, deleteGroupCalls{Pid: pid, Opt: opt})
+						return &gitlab.Response{}, nil
+					},
+				},
+				cr: group(
+					withExternalName("0"),
+					withPermanentlyRemove(gitlab.Ptr(true)),
+					withPath("group"),
+					withFullPathToRemove(gitlab.Ptr("path/to/group")),
+					withStatus(v1alpha1.GroupObservation{FullPath: gitlab.Ptr("path/to/group")})),
+			},
+			want: want{
+				cr: group(
+					withExternalName("0"),
+					withPermanentlyRemove(gitlab.Ptr(true)),
+					withPath("group"),
+					withFullPathToRemove(gitlab.Ptr("path/to/group")),
+					withStatus(v1alpha1.GroupObservation{FullPath: gitlab.Ptr("path/to/group")})),
+				calls: []deleteGroupCalls{
+					{Pid: "0", Opt: &gitlab.DeleteGroupOptions{}},
+					{Pid: "0", Opt: &gitlab.DeleteGroupOptions{PermanentlyRemove: gitlab.Ptr(true), FullPath: gitlab.Ptr("path/to/group")}},
+				},
+				err: nil,
+			},
+		},
+		"SuccessfulPermanentlyTopLevelGroupDeletion": {
+			args: args{
+				group: &fake.MockClient{
+					MockDeleteGroup: func(pid interface{}, opt *gitlab.DeleteGroupOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Response, error) {
+						recordedCalls = append(recordedCalls, deleteGroupCalls{Pid: pid, Opt: opt})
+						return &gitlab.Response{}, nil
+					},
+				},
+				cr: group(
+					withExternalName("0"),
+					withPermanentlyRemove(gitlab.Ptr(true)),
+					withPath("top-level-group"),
+					withStatus(v1alpha1.GroupObservation{FullPath: gitlab.Ptr("top-level-group")})),
+			},
+			want: want{
+				cr: group(
+					withExternalName("0"),
+					withPermanentlyRemove(gitlab.Ptr(true)),
+					withPath("top-level-group"),
+					withStatus(v1alpha1.GroupObservation{FullPath: gitlab.Ptr("top-level-group")}),
+				),
+				calls: []deleteGroupCalls{
+					{Pid: "0", Opt: &gitlab.DeleteGroupOptions{}},
+				},
+				err: nil,
+			},
+		},
 	}
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
+			recordedCalls = nil
 			e := &external{kube: tc.kube, client: tc.group}
 			_, err := e.Delete(context.Background(), tc.args.cr)
 
@@ -902,6 +976,9 @@ func TestDelete(t *testing.T) {
 			if diff := cmp.Diff(tc.want.cr, tc.args.cr, test.EquateConditions()); diff != "" {
 				t.Errorf("r: -want, +got:\n%s", diff)
 			}
+			if diff := cmp.Diff(tc.want.calls, recordedCalls, test.EquateConditions()); diff != "" {
+				t.Errorf("r: -want, +got:\n%s", diff)
+			}
 		})
 	}
 }