Single namespace testing #21
Conversation
WalkthroughIntroduces a JS_NAMESPACE environment variable and replaces hard-coded Kubernetes namespace occurrences with Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
action.yml(1 hunks)tests.bats(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (ubuntu-24.04)
- GitHub Check: test (ubuntu-24.04-arm)
- GitHub Check: test (ubuntu-24.04-arm)
- GitHub Check: test (ubuntu-24.04)
🔇 Additional comments (8)
action.yml (2)
90-91: LGTM! Proper namespace parameterization.The environment variable is correctly exported and will be inherited by the bats tests, which use the same default value.
96-97: Verify that controller deployment creates the "jumpstarter-dev" namespace.The namespace "jumpstarter-dev" is not explicitly created with
kubectl create namespacein action.yml. While the controller deployment step (make -C controller deployon line 72) runs before the serviceaccount creation (lines 96-97), the namespace creation logic resides in the controller repository's deployment configuration, which cannot be verified in the current repository. Confirm that the controller's Makefile or helm chart creates this namespace, or add an explicitkubectl create namespace "${JS_NAMESPACE}"command before lines 96-97.tests.bats (6)
1-2: LGTM! Proper default namespace configuration.The parameter expansion provides a sensible fallback that matches the default set in action.yml, while allowing environment override.
14-19: LGTM! Correct namespace usage in wait commands.The namespace parameter is properly applied to all kubectl wait operations with correct quoting.
26-26: LGTM! Correct namespace in OIDC usernames.The service account OIDC username format correctly incorporates the namespace parameter, following the Kubernetes
system:serviceaccount:NAMESPACE:NAMEpattern.Also applies to: 35-35
46-70: LGTM! Consistent namespace usage across login flows.All
jmp logincommands and token creation operations correctly use the parameterized namespace for both OIDC and service account authentication methods.
145-158: LGTM! Proper namespace usage in client lifecycle tests.The kubectl commands correctly use the parameterized namespace for verifying client existence before and after deletion.
161-174: LGTM! Proper namespace usage in exporter lifecycle tests.The kubectl commands correctly use the parameterized namespace for verifying exporter existence before and after deletion, maintaining consistency with the client tests.
mangelajo
force-pushed
the
single-namespace
branch
from
20addec to
433115c
Compare
tests.bats
Outdated
| @@ -1,3 +1,5 @@ | |||
| JS_NAMESPACE="${JS_NAMESPACE:-jumpstarter-dev}" | |||
There was a problem hiding this comment.
looks like it needs to be created
There was a problem hiding this comment.
it was the wrong one 😶🌫️
mangelajo
force-pushed
the
single-namespace
branch
from
433115c to
c754b59
Compare
mangelajo
force-pushed
the
single-namespace
branch
2 times, most recently
from
151a396 to
495e7bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests.bats (3)
1-1: Ensure the test namespace exists to avoid flakes.Good defaulting of JS_NAMESPACE. Add a quick check in setup() to create it when missing.
setup() { bats_load_library bats-support bats_load_library bats-assert bats_require_minimum_version 1.5.0 + + # Ensure the test namespace exists + kubectl get ns "${JS_NAMESPACE}" >/dev/null 2>&1 || kubectl create ns "${JS_NAMESPACE}" }Also applies to: 3-8
83-104: Background exporter loops run forever; add suite-level cleanup.The three while-true loops keep running beyond this test. Track PIDs and kill them in teardown_file() to prevent leaks across tests/jobs.
+EXPORTER_PIDS=() @test "can run exporters" { cat <<EOF | bash 3>&- & while true; do jmp run --exporter test-exporter-oidc done EOF + EXPORTER_PIDS+=($!) cat <<EOF | bash 3>&- & while true; do jmp run --exporter test-exporter-sa done EOF + EXPORTER_PIDS+=($!) cat <<EOF | bash 3>&- & while true; do jmp run --exporter test-exporter-legacy done EOF + EXPORTER_PIDS+=($!) wait_for_exporter } + +# Stop background exporters once the suite finishes +teardown_file() { + for pid in "${EXPORTER_PIDS[@]}"; do + kill "${pid}" 2>/dev/null || true + done +}
109-115: Env-only flow: clarify JMP_NAME and ensure it truly bypasses saved config.
- JMP_NAME is set to an exporter name; likely should be a client context name (or a unique, non-existent name) to avoid accidentally reusing saved config.
- Since test-client-legacy was created with --save earlier, ensure this command isn’t passing due to config precedence rather than env vars.
- JMP_NAME=test-exporter-legacy \ + # Use a unique, non-existent client context name to force env-only usage + JMP_NAME=env-only-client \ JMP_ENDPOINT=$(kubectl get clients.jumpstarter.dev -n "${JS_NAMESPACE}" test-client-legacy -o 'jsonpath={.status.endpoint}') \ JMP_TOKEN=$(kubectl get secrets -n "${JS_NAMESPACE}" test-client-legacy-client -o 'jsonpath={.data.token}' | base64 -d) \ jmp shell --selector example.com/board=oidc j power onIf the CLI supports disabling config reads (e.g., a --no-config flag), prefer that here for a stricter env-only test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
action.yml(1 hunks)tests.bats(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- action.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (ubuntu-24.04)
- GitHub Check: test (ubuntu-24.04-arm)
🔇 Additional comments (1)
tests.bats (1)
14-19: Namespace parameterization looks consistent.All kubectl/jmp admin/jmp login calls correctly use "${JS_NAMESPACE}". LGTM.
Please confirm action.yml sets/propagates the same JS_NAMESPACE default used here so CI and local runs match.
Also applies to: 23-28, 31-39, 46-71, 141-144
mangelajo
force-pushed
the
single-namespace
branch
from
495e7bc to
8c6191f
Compare
mangelajo
force-pushed
the
single-namespace
branch
from
8c6191f to
635dfbe
Compare
|
Successfully created backport PR for |
We only want to support single-namespace operator for one controller, so we can isolate multiple controllers with their sets of exporters and clients to individual namespaces in a single cluster.
Summary by CodeRabbit
New Features
Tests