Skip to content

Commit

Permalink
This PR was initially to write a test for the ensureNamespaces func…
Browse files Browse the repository at this point in the history
…tion,

not to be confused with the `ensureNamespace` test I have in this commit.

Upon testing `ensureNamespaces`, I think I've come across a few bugs. This
function itself makes a call to `ensureNamespace` and it seems like there
is potentially a scoping issue with where `err` is being set.

It is my understanding that the following pattern:

if err := something(); err != nil {

}

implies that this particular error is available within the scope locally.
You can test out the differece by reverting the small change made in
`mover.go` to see the different in behaviour.

`ensureNamespaces` still needs a test written.

Simplified test code to be more readable

As per the kind suggestions, I have made changes to reflect those
comments in this commit.

Update cmd/clusterctl/client/cluster/mover.go

Co-authored-by: Vince Prignano <vince@vincepri.com>

Updated misleading comment

We're no longer passing extra params here.

Updated comment again

Removed TypeMeta in namespace
  • Loading branch information
hazbo committed Jun 19, 2020
1 parent 3fe965c commit fd43486
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 1 deletion.
3 changes: 2 additions & 1 deletion cmd/clusterctl/client/cluster/mover.go
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,8 @@ func (o *objectMover) ensureNamespace(toProxy Proxy, namespace string) error {
Name: namespace,
}

if err := cs.Get(ctx, key, ns); err == nil {
err = cs.Get(ctx, key, ns)
if err == nil {
return nil
}
if apierrors.IsForbidden(err) {
Expand Down
64 changes: 64 additions & 0 deletions cmd/clusterctl/client/cluster/mover_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -788,3 +788,67 @@ func Test_objectsMoverService_checkTargetProviders(t *testing.T) {
})
}
}

func Test_objectMoverService_ensureNamespace(t *testing.T) {
type args struct {
toProxy Proxy
namespace string
}

namespace1 := &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: "namespace-1",
},
}

tests := []struct {
name string
args args
}{
{
name: "ensureNamespace doesn't fail given an existing namespace",
args: args{
// Create a fake cluster target with namespace-1 already existing
toProxy: test.NewFakeProxy().WithObjs(namespace1),
// Ensure namespace-1 gets created
namespace: "namespace-1",
},
},
{
name: "ensureNamespace doesn't fail if the namespace does not already exist in the target",
args: args{
// Create a fake empty client
toProxy: test.NewFakeProxy(),
// Ensure namespace-2 gets created
namespace: "namespace-2",
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

mover := objectMover{
fromProxy: test.NewFakeProxy(),
}

err := mover.ensureNamespace(tt.args.toProxy, tt.args.namespace)
g.Expect(err).NotTo(HaveOccurred())

// Check that the namespaces either existed or were created in the
// target.
csTo, err := tt.args.toProxy.NewClient()
g.Expect(err).ToNot(HaveOccurred())

ns := &corev1.Namespace{}
key := client.ObjectKey{
// Search for this namespace
Name: tt.args.namespace,
}

err = csTo.Get(ctx, key, ns)
g.Expect(err).ToNot(HaveOccurred())
})
}
}

0 comments on commit fd43486

Please sign in to comment.