-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(bff): Use envtest for kubernetes testing instead of hardcoded mock #490
Conversation
9609468
to
fdca210
Compare
|
||
projectRoot, err := getProjectRoot() | ||
if err != nil { | ||
logger.Error("failed to find project root to locate binaries", err) |
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.
go vet was failing on this line and the other logger.Error
lines due to:
slog.Logger.Error arg "err" should be a string or a slog.Attr (possible missing key or value)
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.
Fixed! But I don't know why I didn't get this error locally! :P
envtest: $(ENVTEST) ## Download setup-envtest locally if necessary. | ||
$(ENVTEST): $(LOCALBIN) | ||
$(call go-install-tool,$(ENVTEST),sigs.k8s.io/controller-runtime/tools/setup-envtest,$(ENVTEST_VERSION)) |
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.
Should the run command try to do this if the mock-k8s-client option is passed to the makefile?
If you try to run make run MOCK_K8S_CLIENT=true MOCK_MR_CLIENT=true
before the binary is installed you get:
time=2024-10-21T15:23:49.306-04:00 level=ERROR msg="failed to start test environment" error="unable to start control plane itself: failed to start the controlplane. retried 5 times: fork/exec /home/gsulliva/Programming/model-registry/clients/ui/bff/bin/k8s/1.29.0-linux-amd64/etcd: no such file or directory"
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.
Nice catch! Fixed!
description := "" | ||
|
||
if service.Annotations != nil { | ||
displayName = service.Annotations["displayName"] |
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.
Would this cause a key error if you had annotations but not displayName
or description
in there?
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.
Nop! If the key doesn't exist, it will return an empty string ("").
Signed-off-by: Eder Ignatowicz <[email protected]>
Signed-off-by: Eder Ignatowicz <[email protected]>
Signed-off-by: Eder Ignatowicz <[email protected]>
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.
Tested locally, works fine!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexcreasy, Griffin-Sullivan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
Use envtest for kubernetes testing instead of hardcoded mock
Description
This PR starts to use envtest for Kubernetes mocking. It also fixes a minor issue when there are no annotations on the service.
How Has This Been Tested?
make run MOCK_K8S_CLIENT=true MOCK_MR_CLIENT=true and also ran the web app:
Merge criteria:
DCO
check)No UI changes.