Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds basic integration tests using k3d clusters and updates dependencies to newer versions. The changes include new test utilities for managing k3d clusters and kubectl operations, test suite configuration, and controller integration tests.
Key Changes:
- Added test infrastructure using k3d for integration testing
- Updated Go modules and Kubernetes dependencies to newer versions
- Migrated from envtest to k3d-based integration tests
- Updated controller code to use newer controller-runtime APIs
Reviewed Changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils/utils.go | Utility functions for running commands in tests |
| tests/utils/kubectl.go | Kubectl wrapper for test operations |
| tests/utils/k3d.go | K3d cluster management utilities |
| tests/suite_test.go | Test suite setup migrated from envtest to k3d |
| tests/controller_test.go | Integration tests for airlock controller |
| tests/assets/ | Test fixtures for MongoDB and airlock resources |
| go.mod | Invalid Go version 1.25.3 specified |
| go.sum | Updated dependency checksums |
| controllers/mongodbcluster_controller.go | Updated to newer controller-runtime API |
| main.go | Commented out metrics and webhook ports |
| config/ | Namespace changed from "system" to "airlock-system" |
| Makefile | Test command updated to use new test suite |
| .github/workflows/ | Invalid Go version 1.25 in golangci-lint workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| _, hasConnectionString := secret.Data["connectionString"] | ||
| if !hasConnectionString { | ||
| return fmt.Errorf("generated secret is missing connectionSecret") |
There was a problem hiding this comment.
Typo in error message: "connectionSecret" should be "connectionString" to match the actual secret field being checked.
| return fmt.Errorf("generated secret is missing connectionSecret") | |
| return fmt.Errorf("generated secret is missing connectionString") |
|
|
||
| Context("Airlock Controller", func() { | ||
| It("should run successfully", func() { | ||
| // FIXME: this is failig -_- |
There was a problem hiding this comment.
Typo in comment: "failig" should be "failing".
| // FIXME: this is failig -_- | |
| // FIXME: this is failing -_- |
| // MetricsBindAddress: metricsAddr, | ||
| // Port: 9443, |
There was a problem hiding this comment.
why commenting these out?
geekgonecrazy
left a comment
geekgonecrazy
left a comment
There was a problem hiding this comment.
Over all seems like a good improvement to be doing more integration testing and seeing how this thing will actually behave in a kubernetes cluster
| - --leader-elect | ||
| image: controller:latest | ||
| imagePullPolicy: Always | ||
| image: docker.io/library/controller:latest |
There was a problem hiding this comment.
Will revert. Was checking something.
There was a problem hiding this comment.
Will revert. Was checking something.
| image: controller:latest | ||
| imagePullPolicy: Always | ||
| image: docker.io/library/controller:latest | ||
| imagePullPolicy: IfNotPresent |
There was a problem hiding this comment.
Pull policy yes needs changing because I'm importing image to k3d, and always policy will not use that image but try to pull
There was a problem hiding this comment.
Pull policy yes needs changing because I'm importing image to k3d, and always policy will not use that image but try to pull
There was a problem hiding this comment.
Pull policy yes needs changing because I'm importing image to k3d, and always policy will not use that image but try to pull
| metadata: | ||
| name: controller-manager | ||
| namespace: system | ||
| namespace: airlock-system |
There was a problem hiding this comment.
I don't get why those changes to the namespace is needed.
In config/default/kustomization.yaml, there is
namespace: airlock-system
namePrefix: airlock-
so all resources here will go to that namespace, and be named correctly.....
There was a problem hiding this comment.
I'll revert. Did this before saw the kustomize file
| app.kubernetes.io/part-of: airlock | ||
| app.kubernetes.io/managed-by: kustomize | ||
| name: system | ||
| name: airlock-system |
There was a problem hiding this comment.
Yeah, this will break, it will be called airlock-airlock-system
|
|
||
| ready := false | ||
|
|
||
| // TODO: i doubt this is full proof |
There was a problem hiding this comment.
should we look in mongo to see if it actually created? or do we just trust ourselves?
| return err | ||
| } | ||
|
|
||
| _, hasConnectionString := secret.Data["connectionString"] |
There was a problem hiding this comment.
same here, should we try to connect?
3 participants
No description provided.