Skip to content

Commit

Permalink
CI-1658: Address review comments
Browse files Browse the repository at this point in the history
- Add `asdf` `.tool-versions` file which specifies go version to be used
  for local development for `asdf` users.
- Add comment to Makefile specifying how to install and configure
  required dependencies for running `make test` locally.
- Replace all `time.Sleep` with `Eventually` in order to give asynchronous
  functions time to create their intended resources.
- Change `shortTimeout` to 500 milliseconds.
  • Loading branch information
Nabil372 committed Dec 16, 2022
1 parent 5442a88 commit c1b5d84
Show file tree
Hide file tree
Showing 6 changed files with 16 additions and 19 deletions.
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,3 @@
*.coverprofile
/dist/
.vscode
.tool-versions
1 change: 1 addition & 0 deletions .tool-versions
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
golang 1.19.3
2 changes: 2 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ bin/%.darwin_amd64:
bin/%:
CGO_ENABLED=0 GOARCH=amd64 $(BUILD_COMMAND) -o $@ ./cmd/$*/.

# Run the below commands in order to install the required dependencies for
# running `make test` locally.
# go install github.com/onsi/ginkgo/ginkgo@v1.16.5
# go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest
# setup-envtest use -p path 1.22.x
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package integration

import (
"context"
"time"

corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
Expand Down Expand Up @@ -103,15 +102,14 @@ var _ = Describe("Reconciler", func() {
HaveOccurred(), "failed to create 'foo' DirectoryRoleBinding",
)

// Wait to ensure that the apiserver has time to create DirectoryRoleBinding
time.Sleep(1 * time.Second)

By("Validate associated RoleBinding exists")
rb := &rbacv1.RoleBinding{}
identifier := client.ObjectKeyFromObject(drb)
err := mgr.GetClient().Get(context.TODO(), identifier, rb)

Expect(err).NotTo(HaveOccurred(), "failed to find associated RoleBinding for DirectoryRoleBinding")
Eventually(func() error {
return mgr.GetClient().Get(context.TODO(), identifier, rb)
}).ShouldNot(HaveOccurred(), "failed to find associated RoleBinding for DirectoryRoleBinding")

Expect(rb.RoleRef).To(Equal(drb.Spec.RoleRef), "associated RoleBinding should reference same Role as DRB")
Expect(rb.Subjects).To(BeEmpty(), "initial RoleBinding should contain no subjects")

Expand All @@ -124,7 +122,7 @@ var _ = Describe("Reconciler", func() {
newUser("manuel@gocardless.com"),
}

err = mgr.GetClient().Update(context.TODO(), drb)
err := mgr.GetClient().Update(context.TODO(), drb)
Expect(err).NotTo(HaveOccurred(), "failed to update DirectoryRoleBinding")

By("Refresh RoleBinding")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -334,16 +334,13 @@ var _ = Describe("Console", func() {
return err
}).ShouldNot(HaveOccurred(), "failed to update console")

// Wait for a second to ensure that the apiserver has time to create a job if it were to do so.
time.Sleep(1 * time.Second)

By("Check that only one job has been created")
Eventually(func() []batchv1.Job {
jobs := &batchv1.JobList{}
err := mgr.GetClient().List(context.TODO(), jobs, client.InNamespace(identifier.Namespace))
Expect(err).NotTo(HaveOccurred())
return jobs.Items
}).Should(HaveLen(1), "Only 1 job should be created")
}, 2*time.Second).Should(HaveLen(1), "Only 1 job should be created")
})

It("Creates a role and directory role binding for the user", func() {
Expand Down
14 changes: 7 additions & 7 deletions pkg/workloads/console/runner/integration/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func newNamespace(name string) corev1.Namespace {
}

const (
shortTimeout time.Duration = 30 * time.Millisecond
shortTimeout time.Duration = 500 * time.Millisecond
)

func newConsoleTemplate(namespace, name string, labels map[string]string) workloadsv1alpha1.ConsoleTemplate {
Expand Down Expand Up @@ -119,9 +119,9 @@ func mustCreateRoleBinding(roleBinding rbacv1.RoleBinding) {

func mustAddSubjectsToRoleBinding(rb rbacv1.RoleBinding, subjects []rbacv1.Subject) {
rb.Subjects = subjects
err := kubeClient.Update(context.TODO(), &rb)
time.Sleep(2 * time.Second)
Expect(err).NotTo(HaveOccurred())
Eventually(func() error {
return kubeClient.Update(context.TODO(), &rb)
}, 2*time.Second).ShouldNot(HaveOccurred())
}

func mustUpdateConsolePhase(in workloadsv1alpha1.Console, phase workloadsv1alpha1.ConsolePhase) {
Expand Down Expand Up @@ -281,7 +281,7 @@ var _ = Describe("Runner", func() {

Context("When console phase is Pending", func() {
It("Fails with a timeout waiting", func() {
ctx, cancel := context.WithTimeout(context.Background(), shortTimeout+time.Second)
ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
defer cancel()
_, err := consoleRunner.WaitUntilReady(ctx, console, true)

Expand Down Expand Up @@ -436,7 +436,7 @@ var _ = Describe("Runner", func() {
rb.Subjects = nil
mustCreateRoleBinding(rb)

ctx, cancel := context.WithTimeout(context.Background(), shortTimeout+time.Second)
ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
defer cancel()
_, err := consoleRunner.WaitUntilReady(ctx, csl, true)

Expand Down Expand Up @@ -487,7 +487,7 @@ var _ = Describe("Runner", func() {
},
)

ctx, cancel := context.WithTimeout(context.Background(), shortTimeout+time.Second)
ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
defer cancel()
_, err := consoleRunner.WaitUntilReady(ctx, csl, true)

Expand Down

0 comments on commit c1b5d84

Please sign in to comment.