feat: Implement SpaceBindingRequest controller main logic #779
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,267 @@ | ||||||||||
| package parallel | ||||||||||
|
|
||||||||||
| import ( | ||||||||||
| "context" | ||||||||||
| "fmt" | ||||||||||
| "testing" | ||||||||||
| "time" | ||||||||||
|
|
||||||||||
| toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" | ||||||||||
| "github.com/gofrs/uuid" | ||||||||||
|
|
||||||||||
| spacebindingrequesttestcommon "github.com/codeready-toolchain/toolchain-common/pkg/test/spacebindingrequest" | ||||||||||
|
|
||||||||||
| testspace "github.com/codeready-toolchain/toolchain-common/pkg/test/space" | ||||||||||
| . "github.com/codeready-toolchain/toolchain-e2e/testsupport" | ||||||||||
| . "github.com/codeready-toolchain/toolchain-e2e/testsupport/wait" | ||||||||||
| "github.com/stretchr/testify/require" | ||||||||||
| "k8s.io/apimachinery/pkg/types" | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| func TestCreateSpaceBindingRequest(t *testing.T) { | ||||||||||
| // given | ||||||||||
| t.Parallel() | ||||||||||
| // make sure everything is ready before running the actual tests | ||||||||||
| awaitilities := WaitForDeployments(t) | ||||||||||
| hostAwait := awaitilities.Host() | ||||||||||
| memberAwait := awaitilities.Member1() | ||||||||||
|
|
||||||||||
| t.Run("success", func(t *testing.T) { | ||||||||||
| t.Run("create space binding request", func(t *testing.T) { | ||||||||||
| // when | ||||||||||
| // we create a space to share , a new MUR and a SpaceBindingRequest | ||||||||||
| space, spaceBindingRequest, spaceBinding := NewSpaceBindingRequest(t, awaitilities, memberAwait, hostAwait, "admin") | ||||||||||
|
There was a problem hiding this comment.
Suggested change
There was a problem hiding this comment. maybe
Suggested change
There was a problem hiding this comment. I named it this way because we also have There was a problem hiding this comment. Another alternative could be: There was a problem hiding this comment. what about There was a problem hiding this comment.
|
||||||||||
|
|
||||||||||
| t.Run("spaceBinding is recreated if deleted ", func(t *testing.T) { | ||||||||||
| // now, delete the SpaceBinding, | ||||||||||
| // a new SpaceBinding will be provisioned by the SpaceBindingRequest. | ||||||||||
| // | ||||||||||
| // save the creation timestamp that will be used to ensure that a new SpaceBinding was created with the same name. | ||||||||||
| oldSpaceCreationTimeStamp := spaceBinding.CreationTimestamp | ||||||||||
| // save the old UID that will be used to ensure that a new SpaceBinding was created with the same name but new UID | ||||||||||
| oldUID := spaceBinding.UID | ||||||||||
|
|
||||||||||
| // when | ||||||||||
| err := hostAwait.Client.Delete(context.TODO(), spaceBinding) | ||||||||||
|
|
||||||||||
| // then | ||||||||||
| // a new SpaceBinding is created | ||||||||||
| // with the same name but creation timestamp should be greater (more recent). | ||||||||||
| require.NoError(t, err) | ||||||||||
| spaceBinding, err = hostAwait.WithRetryOptions(TimeoutOption(time.Second*10), RetryInterval(time.Second*2)).WaitForSpaceBinding(t, spaceBindingRequest.Spec.MasterUserRecord, space.Name, | ||||||||||
|
There was a problem hiding this comment. is there any reason for using the different retry options?
There was a problem hiding this comment. I increased the retry interval and timeout because I'm noticing the test fails from time to time with this error : full logs here maybe I can remove now the retry interval and see if it's more stable, or at least add a comment to explain why is there. WDYT? There was a problem hiding this comment. Ok, I've noticed after that the default timeout was actually bigger than what I was setting, so I've removed that and also removed the |
||||||||||
| UntilSpaceBindingHasMurName(spaceBindingRequest.Spec.MasterUserRecord), | ||||||||||
| UntilSpaceBindingHasSpaceName(space.Name), | ||||||||||
| UntilSpaceBindingHasSpaceRole(spaceBindingRequest.Spec.SpaceRole), | ||||||||||
| UntilSpaceBindingHasCreationTimestampGreaterThan(oldSpaceCreationTimeStamp.Time), | ||||||||||
| UntilSpaceBindingHasDifferentUID(oldUID), | ||||||||||
| UntilSpaceBindingHasLabel(toolchainv1alpha1.SpaceBindingRequestLabelKey, spaceBindingRequest.GetName()), | ||||||||||
| UntilSpaceBindingHasLabel(toolchainv1alpha1.SpaceBindingRequestNamespaceLabelKey, spaceBindingRequest.GetNamespace()), | ||||||||||
| ) | ||||||||||
| require.NoError(t, err) | ||||||||||
|
|
||||||||||
| t.Run("SpaceBinding always reflects values from spaceBindingRequest ", func(t *testing.T) { | ||||||||||
| // given | ||||||||||
| // something/someone updates the SpaceRole directly on the SpaceBinding object | ||||||||||
|
|
||||||||||
| // when | ||||||||||
| spaceBinding, err = hostAwait.UpdateSpaceBinding(t, spaceBinding.Name, func(s *toolchainv1alpha1.SpaceBinding) { | ||||||||||
| s.Spec.SpaceRole = "invalidRole" // let's change the role | ||||||||||
| }) | ||||||||||
| require.NoError(t, err) | ||||||||||
|
|
||||||||||
| // then | ||||||||||
| // spaceBindingRequest should reset back the SpaceRole | ||||||||||
| spaceBinding, err = hostAwait.WaitForSpaceBinding(t, spaceBindingRequest.Spec.MasterUserRecord, space.Name, | ||||||||||
| UntilSpaceBindingHasMurName(spaceBindingRequest.Spec.MasterUserRecord), | ||||||||||
| UntilSpaceBindingHasSpaceName(space.Name), | ||||||||||
| UntilSpaceBindingHasSpaceRole(spaceBindingRequest.Spec.SpaceRole), // should have back the role from the SBR | ||||||||||
| UntilSpaceBindingHasLabel(toolchainv1alpha1.SpaceBindingRequestLabelKey, spaceBindingRequest.GetName()), | ||||||||||
| UntilSpaceBindingHasLabel(toolchainv1alpha1.SpaceBindingRequestNamespaceLabelKey, spaceBindingRequest.GetNamespace()), | ||||||||||
| ) | ||||||||||
| require.NoError(t, err) | ||||||||||
|
|
||||||||||
| t.Run("delete space binding request", func(t *testing.T) { | ||||||||||
| // now, delete the SpaceBindingRequest and expect that the SpaceBinding will be deleted as well, | ||||||||||
|
|
||||||||||
| // when | ||||||||||
| err := memberAwait.Client.Delete(context.TODO(), spaceBindingRequest) | ||||||||||
|
|
||||||||||
| // then | ||||||||||
| // spaceBinding should be deleted as well | ||||||||||
| require.NoError(t, err) | ||||||||||
| err = hostAwait.WaitUntilSpaceBindingDeleted(spaceBinding.Name) | ||||||||||
| require.NoError(t, err) | ||||||||||
| }) | ||||||||||
| }) | ||||||||||
| }) | ||||||||||
| }) | ||||||||||
| }) | ||||||||||
|
|
||||||||||
| t.Run("error", func(t *testing.T) { | ||||||||||
| t.Run("unable create space binding request with invalid SpaceRole", func(t *testing.T) { | ||||||||||
| space, _, _ := CreateSpace(t, awaitilities, testspace.WithTierName("appstudio"), testspace.WithSpecTargetCluster(memberAwait.ClusterName)) | ||||||||||
| // wait for the namespace to be provisioned since we will be creating the SpaceBindingRequest into it. | ||||||||||
| space, err := hostAwait.WaitForSpace(t, space.Name, UntilSpaceHasAnyProvisionedNamespaces()) | ||||||||||
| require.NoError(t, err) | ||||||||||
|
Comment on lines
+101
to
+103
There was a problem hiding this comment. the same - no need for another wait
Suggested change
|
||||||||||
| // let's create a new MUR that will have access to the space | ||||||||||
| username := uuid.Must(uuid.NewV4()).String() | ||||||||||
| _, mur := NewSignupRequest(awaitilities). | ||||||||||
| Username(username). | ||||||||||
| Email(username + "@acme.com"). | ||||||||||
| ManuallyApprove(). | ||||||||||
| TargetCluster(memberAwait). | ||||||||||
| RequireConditions(ConditionSet(Default(), ApprovedByAdmin())...). | ||||||||||
| NoSpace(). | ||||||||||
| WaitForMUR().Execute(t).Resources() | ||||||||||
| // create the spacebinding request | ||||||||||
| spaceBindingRequest := CreateSpaceBindingRequest(t, awaitilities, memberAwait.ClusterName, | ||||||||||
| WithSpecSpaceRole("invalid"), // set invalid spacerole | ||||||||||
| WithSpecMasterUserRecord(mur.GetName()), | ||||||||||
| WithNamespace(GetDefaultNamespace(space.Status.ProvisionedNamespaces)), | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| // then | ||||||||||
| require.NoError(t, err) | ||||||||||
| // wait for spacebinding request status to be set | ||||||||||
| _, err = memberAwait.WaitForSpaceBindingRequest(t, types.NamespacedName{Namespace: spaceBindingRequest.GetNamespace(), Name: spaceBindingRequest.GetName()}, | ||||||||||
| UntilSpaceBindingRequestHasConditions(spacebindingrequesttestcommon.UnableToCreateSpaceBinding(fmt.Sprintf("invalid role 'invalid' for space '%s'", space.Name))), | ||||||||||
| ) | ||||||||||
| require.NoError(t, err) | ||||||||||
mfrancisc marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
| }) | ||||||||||
|
|
||||||||||
| t.Run("unable create space binding request with invalid MasterUserRecord", func(t *testing.T) { | ||||||||||
| space, _, _ := CreateSpace(t, awaitilities, testspace.WithTierName("appstudio"), testspace.WithSpecTargetCluster(memberAwait.ClusterName)) | ||||||||||
| // wait for the namespace to be provisioned since we will be creating the SpaceBindingRequest into it. | ||||||||||
| space, err := hostAwait.WaitForSpace(t, space.Name, UntilSpaceHasAnyProvisionedNamespaces()) | ||||||||||
| require.NoError(t, err) | ||||||||||
| // create the spacebinding request | ||||||||||
| spaceBindingRequest := CreateSpaceBindingRequest(t, awaitilities, memberAwait.ClusterName, | ||||||||||
| WithSpecSpaceRole("admin"), | ||||||||||
| WithSpecMasterUserRecord("invalidMUR"), // we set an invalid MUR | ||||||||||
| WithNamespace(GetDefaultNamespace(space.Status.ProvisionedNamespaces)), | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| // then | ||||||||||
| require.NoError(t, err) | ||||||||||
| // wait for spacebinding request status to be set | ||||||||||
| _, err = memberAwait.WaitForSpaceBindingRequest(t, types.NamespacedName{Namespace: spaceBindingRequest.GetNamespace(), Name: spaceBindingRequest.GetName()}, | ||||||||||
| UntilSpaceBindingRequestHasConditions(spacebindingrequesttestcommon.UnableToCreateSpaceBinding("unable to get MUR: MasterUserRecord.toolchain.dev.openshift.com \"invalidMUR\" not found")), | ||||||||||
| ) | ||||||||||
| require.NoError(t, err) | ||||||||||
mfrancisc marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
| }) | ||||||||||
| }) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func TestUpdateSpaceBindingRequest(t *testing.T) { | ||||||||||
| // given | ||||||||||
| t.Parallel() | ||||||||||
| // make sure everything is ready before running the actual tests | ||||||||||
| awaitilities := WaitForDeployments(t) | ||||||||||
| hostAwait := awaitilities.Host() | ||||||||||
| memberAwait := awaitilities.Member1() | ||||||||||
|
|
||||||||||
| t.Run("update space binding request SpaceRole", func(t *testing.T) { | ||||||||||
| // when | ||||||||||
| space, spaceBindingRequest, _ := NewSpaceBindingRequest(t, awaitilities, memberAwait, hostAwait, "contributor") | ||||||||||
| _, err := memberAwait.UpdateSpaceBindingRequest(t, types.NamespacedName{Namespace: spaceBindingRequest.Namespace, Name: spaceBindingRequest.Name}, | ||||||||||
| func(s *toolchainv1alpha1.SpaceBindingRequest) { | ||||||||||
| s.Spec.SpaceRole = "admin" // set to admin from contributor | ||||||||||
| }, | ||||||||||
| ) | ||||||||||
| require.NoError(t, err) | ||||||||||
|
|
||||||||||
| //then | ||||||||||
| // wait for both SpaceBindingRequest and SpaceBinding to have same SpaceRole | ||||||||||
| spaceBindingRequest, err = memberAwait.WaitForSpaceBindingRequest(t, types.NamespacedName{Namespace: spaceBindingRequest.GetNamespace(), Name: spaceBindingRequest.GetName()}, | ||||||||||
| UntilSpaceBindingRequestHasConditions(spacebindingrequesttestcommon.Ready()), | ||||||||||
| UntilSpaceBindingRequestHasSpecSpaceRole("admin"), // has admin role | ||||||||||
| UntilSpaceBindingRequestHasSpecMUR(spaceBindingRequest.Spec.MasterUserRecord), | ||||||||||
| ) | ||||||||||
| require.NoError(t, err) | ||||||||||
| _, err = hostAwait.WaitForSpaceBinding(t, spaceBindingRequest.Spec.MasterUserRecord, space.Name, | ||||||||||
| UntilSpaceBindingHasMurName(spaceBindingRequest.Spec.MasterUserRecord), | ||||||||||
| UntilSpaceBindingHasSpaceName(space.Name), | ||||||||||
| UntilSpaceBindingHasSpaceRole("admin"), // has admin role | ||||||||||
| UntilSpaceBindingHasLabel(toolchainv1alpha1.SpaceBindingRequestLabelKey, spaceBindingRequest.GetName()), | ||||||||||
| UntilSpaceBindingHasLabel(toolchainv1alpha1.SpaceBindingRequestNamespaceLabelKey, spaceBindingRequest.GetNamespace()), | ||||||||||
| ) | ||||||||||
| require.NoError(t, err) | ||||||||||
| }) | ||||||||||
|
|
||||||||||
| t.Run("update space binding request MasterUserRecord", func(t *testing.T) { | ||||||||||
|
There was a problem hiding this comment. I'm wondering how much we want to support this scenario. I would love to make the MUR immutable, which would require an admission webhook of course... There was a problem hiding this comment. Makes sense, I haven't considered that the name doesn't change! There was a problem hiding this comment. sure, that was more as a follow up task |
||||||||||
| // when | ||||||||||
| space, spaceBindingRequest, _ := NewSpaceBindingRequest(t, awaitilities, memberAwait, hostAwait, "admin") | ||||||||||
| // let's create another MUR that will have access to the space | ||||||||||
| username := uuid.Must(uuid.NewV4()).String() | ||||||||||
| _, newmur := NewSignupRequest(awaitilities). | ||||||||||
| Username(username). | ||||||||||
| Email(username + "@acme.com"). | ||||||||||
| ManuallyApprove(). | ||||||||||
| TargetCluster(memberAwait). | ||||||||||
| RequireConditions(ConditionSet(Default(), ApprovedByAdmin())...). | ||||||||||
| NoSpace(). | ||||||||||
| WaitForMUR().Execute(t).Resources() | ||||||||||
| // and we update the MUR in the SBR | ||||||||||
| _, err := memberAwait.UpdateSpaceBindingRequest(t, types.NamespacedName{Namespace: spaceBindingRequest.Namespace, Name: spaceBindingRequest.Name}, | ||||||||||
| func(s *toolchainv1alpha1.SpaceBindingRequest) { | ||||||||||
| s.Spec.MasterUserRecord = newmur.GetName() // set to the new MUR | ||||||||||
| }, | ||||||||||
| ) | ||||||||||
| require.NoError(t, err) | ||||||||||
|
|
||||||||||
| //then | ||||||||||
| // wait for both SpaceBindingRequest and SpaceBinding to have same MUR | ||||||||||
| spaceBindingRequest, err = memberAwait.WaitForSpaceBindingRequest(t, types.NamespacedName{Namespace: spaceBindingRequest.GetNamespace(), Name: spaceBindingRequest.GetName()}, | ||||||||||
| UntilSpaceBindingRequestHasConditions(spacebindingrequesttestcommon.Ready()), | ||||||||||
| UntilSpaceBindingRequestHasSpecSpaceRole(spaceBindingRequest.Spec.SpaceRole), | ||||||||||
| UntilSpaceBindingRequestHasSpecMUR(newmur.GetName()), // new MUR | ||||||||||
| ) | ||||||||||
| require.NoError(t, err) | ||||||||||
| _, err = hostAwait.WaitForSpaceBinding(t, spaceBindingRequest.Spec.MasterUserRecord, space.Name, | ||||||||||
| UntilSpaceBindingHasMurName(newmur.GetName()), // has new MUR | ||||||||||
| UntilSpaceBindingHasSpaceName(space.Name), | ||||||||||
| UntilSpaceBindingHasSpaceRole(spaceBindingRequest.Spec.SpaceRole), | ||||||||||
| ) | ||||||||||
| require.NoError(t, err) | ||||||||||
| }) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func NewSpaceBindingRequest(t *testing.T, awaitilities Awaitilities, memberAwait *MemberAwaitility, hostAwait *HostAwaitility, spaceRole string) (*toolchainv1alpha1.Space, *toolchainv1alpha1.SpaceBindingRequest, *toolchainv1alpha1.SpaceBinding) { | ||||||||||
| space, _, _ := CreateSpace(t, awaitilities, testspace.WithTierName("appstudio"), testspace.WithSpecTargetCluster(memberAwait.ClusterName)) | ||||||||||
| // wait for the namespace to be provisioned since we will be creating the SpaceBindingRequest into it. | ||||||||||
| space, err := hostAwait.WaitForSpace(t, space.Name, UntilSpaceHasAnyProvisionedNamespaces()) | ||||||||||
| require.NoError(t, err) | ||||||||||
|
Comment on lines
+235
to
+237
There was a problem hiding this comment. the _, err = memberAwait.WaitForNSTmplSet(t, space.Name,
wait.UntilNSTemplateSetHasSpaceRoles(
wait.SpaceRole(tier.Spec.SpaceRoles["admin"].TemplateRef, mur.Name)))so we can assume that it's ready for new SpaceBindings to be created
Suggested change
There was a problem hiding this comment. I see your point, but the reason I added this wait , is because even when the NSTemplateSet was ready the but maybe I'm doing it in the wrong way? There was a problem hiding this comment. ah, makes sense, I missed that part. Thanks for explanation 👍 |
||||||||||
| // let's create a new MUR that will have access to the space | ||||||||||
| username := uuid.Must(uuid.NewV4()).String() | ||||||||||
| _, mur := NewSignupRequest(awaitilities). | ||||||||||
| Username(username). | ||||||||||
| Email(username + "@acme.com"). | ||||||||||
| ManuallyApprove(). | ||||||||||
| TargetCluster(memberAwait). | ||||||||||
| RequireConditions(ConditionSet(Default(), ApprovedByAdmin())...). | ||||||||||
| NoSpace(). | ||||||||||
| WaitForMUR().Execute(t).Resources() | ||||||||||
| // create the spacebinding request | ||||||||||
| spaceBindingRequest := CreateSpaceBindingRequest(t, awaitilities, memberAwait.ClusterName, | ||||||||||
| WithSpecSpaceRole(spaceRole), | ||||||||||
| WithSpecMasterUserRecord(mur.GetName()), | ||||||||||
| WithNamespace(GetDefaultNamespace(space.Status.ProvisionedNamespaces)), | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| // then | ||||||||||
| // check for the spaceBinding creation | ||||||||||
| spaceBinding, err := hostAwait.WaitForSpaceBinding(t, spaceBindingRequest.Spec.MasterUserRecord, space.Name, | ||||||||||
| UntilSpaceBindingHasMurName(spaceBindingRequest.Spec.MasterUserRecord), | ||||||||||
| UntilSpaceBindingHasSpaceName(space.Name), | ||||||||||
| UntilSpaceBindingHasSpaceRole(spaceBindingRequest.Spec.SpaceRole), | ||||||||||
| UntilSpaceBindingHasLabel(toolchainv1alpha1.SpaceBindingRequestLabelKey, spaceBindingRequest.GetName()), | ||||||||||
| UntilSpaceBindingHasLabel(toolchainv1alpha1.SpaceBindingRequestNamespaceLabelKey, spaceBindingRequest.GetNamespace()), | ||||||||||
| ) | ||||||||||
| require.NoError(t, err) | ||||||||||
| // wait for spacebinding request status | ||||||||||
| spaceBindingRequest, err = memberAwait.WaitForSpaceBindingRequest(t, types.NamespacedName{Namespace: spaceBindingRequest.GetNamespace(), Name: spaceBindingRequest.GetName()}, | ||||||||||
| UntilSpaceBindingRequestHasConditions(spacebindingrequesttestcommon.Ready()), | ||||||||||
| ) | ||||||||||
| require.NoError(t, err) | ||||||||||
|
There was a problem hiding this comment. but we could maybe check that everything was propagated to the NSTemplateSet correctly here, WDYT? if spaceRole == "admin" {
_, err = memberAwait.WaitForNSTmplSet(t, space.Name,
UntilNSTemplateSetHasSpaceRoles(
SpaceRole(tier.Spec.SpaceRoles[spaceRole].TemplateRef, userSignup.Status.CompliantUsername, userSignup.mur.Name)))
} else {
_, err = memberAwait.WaitForNSTmplSet(t, space.Name,
UntilNSTemplateSetHasSpaceRoles(
SpaceRole(tier.Spec.SpaceRoles["admin"].TemplateRef, userSignup.Status.CompliantUsername),
SpaceRole(tier.Spec.SpaceRoles[spaceRole].TemplateRef, userSignup.mur.Name)))
}
VerifyResourcesProvisionedForSpace(t, awaitilities, space.Name)you know - to be sure that the SpaceBinding that we created will be properly processed by the Space controller There was a problem hiding this comment. Ok I see what you meant , but I'm not 100% sure I understood your example code - why do we need to do it this way? If I'm not mistaken the usernames added to the NSTemplateSet are the just the So in theory this could be just: but I'm sure I'm missing something. There was a problem hiding this comment. I now understand why we need todo it this way, it's because we have 2 murs 🤦 . |
||||||||||
| return space, spaceBindingRequest, spaceBinding | ||||||||||
| } | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| package testsupport | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" | ||
| "github.com/codeready-toolchain/toolchain-e2e/testsupport/wait" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| type SpaceBindingRequestOption func(request *toolchainv1alpha1.SpaceBindingRequest) | ||
|
|
||
| func WithSpecSpaceRole(spaceRole string) SpaceBindingRequestOption { | ||
| return func(s *toolchainv1alpha1.SpaceBindingRequest) { | ||
| s.Spec.SpaceRole = spaceRole | ||
| } | ||
| } | ||
|
|
||
| func WithSpecMasterUserRecord(mur string) SpaceBindingRequestOption { | ||
| return func(s *toolchainv1alpha1.SpaceBindingRequest) { | ||
| s.Spec.MasterUserRecord = mur | ||
| } | ||
| } | ||
|
|
||
| func WithNamespace(ns string) SpaceBindingRequestOption { | ||
|
There was a problem hiding this comment. wouldn't be There was a problem hiding this comment. right , that is how I wanted to name it too but that function already exists for SpaceRequests in the same package: toolchain-e2e/testsupport/spacerequest.go Lines 37 to 41 in 82d14a8 Maybe I can make this more generic and use it for both ? There was a problem hiding this comment. Alternatively we could group the files under
but I suggest we do this in a separate PR. There was a problem hiding this comment. I like the Something like: hence, we can have the same There was a problem hiding this comment. that being said, having a sub package for each type is fine and probably easier to do, given the current codebase! There was a problem hiding this comment. @xcoulon thanks a lot! indeed your pattern proposal is another option. I don't have a strong opinion, even if looking at it probably the package segregation would be "easier" ( just changing the import statements ), while this new pattern I'm not sure I saw it used anywhere else in our code ? My proposal is to decide which path we want to go and I'll address it in a follow up PR. WDYT ? There was a problem hiding this comment. I believe that splitting (or segregating) the code in There was a problem hiding this comment. Ok, thanks ! I'm adding that to my backlog and will come up with a PR once I've merged the existing ones. |
||
| return func(s *toolchainv1alpha1.SpaceBindingRequest) { | ||
| s.ObjectMeta.Namespace = ns | ||
| } | ||
| } | ||
|
|
||
| func CreateSpaceBindingRequest(t *testing.T, awaitilities wait.Awaitilities, memberName string, opts ...SpaceBindingRequestOption) *toolchainv1alpha1.SpaceBindingRequest { | ||
| memberAwait, err := awaitilities.Member(memberName) | ||
| require.NoError(t, err) | ||
| namePrefix := NewObjectNamePrefix(t) | ||
|
|
||
| spaceBindingRequest := &toolchainv1alpha1.SpaceBindingRequest{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| GenerateName: namePrefix + "-", | ||
| }, | ||
| } | ||
| for _, apply := range opts { | ||
| apply(spaceBindingRequest) | ||
| } | ||
| err = memberAwait.CreateWithCleanup(t, spaceBindingRequest) | ||
| require.NoError(t, err) | ||
|
|
||
| return spaceBindingRequest | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be executed before deleting Spaces. Deletion of Spaces should trigger deletion of everything in the namespaces, including SBR. That being said - do we need to execute the deletion of SRs and SBRs explicitly at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right , we don't need to delete
spacebindingrequests, but we do need to deletespacerequestsbecause the spaces created by those cannot be deleted without deleting their SR resource.I'll remove this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the deletion of
spacebindingrequestsin 9fda3e4