LCORE-675: rename module auth to authentication to distinguish it from authorization #538
Conversation
WalkthroughProject-wide rename of imports from auth.* to authentication.* across endpoints, authentication modules, and tests; pyproject.toml pyright exclude updated to the new path; one non-functional comment added in lightspeed_stack.py. No behavioral or public API changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 2
🧹 Nitpick comments (20)
pyproject.toml (1)
28-28: Nit: update comment to match new path.Apply:
- # Used by auth/k8s integration + # Used by authentication/k8s integrationsrc/authentication/noop.py (1)
13-13: Optional: use AuthTuple in type hints for consistency.Small polish to reference the canonical alias.
-from authentication.interface import AuthInterface +from authentication.interface import AuthInterface, AuthTuple @@ - async def __call__(self, request: Request) -> tuple[str, str, bool, str]: + async def __call__(self, request: Request) -> AuthTuple:Also applies to: 26-26
tests/unit/authentication/test_jwk_token.py (1)
3-3: Nit: docstring still references auth path.-"""Unit tests for functions defined in auth/jwk_token.py""" +"""Unit tests for functions defined in authentication/jwk_token.py"""tests/unit/authentication/test_utils.py (1)
1-1: Nit: docstring still references auth path.-"""Unit tests for functions defined in auth/utils.py""" +"""Unit tests for functions defined in authentication/utils.py"""src/authentication/noop_with_token.py (1)
21-22: Docstring drift: update references to new package path and returned tuple.The module docstring still points to
auth.utils.extract_user_token, and the method docstring no longer reflects the 4‑tuple returned.Apply:
@@ - - Reads a user token from request headers via `auth.utils.extract_user_token`. + - Reads a user token from request headers via `authentication.utils.extract_user_token`. @@ - Returns: - The user's UID and username if authentication and authorization succeed - user_id check is skipped with noop auth to allow consumers provide user_id + Returns: + tuple[str, str, bool, str]: (user_id, DEFAULT_USER_NAME, skip_userid_check, user_token)src/app/endpoints/streaming_query.py (2)
23-24: Set SSE content type on StreamingResponse.Without
media_type="text/event-stream", some clients may not process the stream correctly.Apply:
- return StreamingResponse(response_generator(response)) + return StreamingResponse( + response_generator(response), + media_type="text/event-stream", + )
23-24: Sanity check — leftoverauth/*docstrings & config (no activeauth.*imports found).No Python imports of
auth.*were found; repo contains string/docstring/config references toauth/*— update toauthentication/*or confirm intentional.
- src/authentication/noop_with_token.py:6 — Reads a user token from request headers via
auth.utils.extract_user_token.- pyproject.toml:28 — # Used by auth/k8s integration
- pyproject.toml:59 — # service/ols/src/auth/k8s.py and currently has 58 Pyright issues. It
- tests/unit/authentication/test_utils.py:1 — """Unit tests for functions defined in auth/utils.py"""
- tests/unit/authentication/test_noop_with_token.py:1 — """Unit tests for functions defined in auth/noop_with_token.py"""
- tests/unit/authentication/test_noop.py:1 — """Unit tests for functions defined in auth/noop.py"""
- tests/unit/authentication/test_jwk_token.py:3 — """Unit tests for functions defined in auth/jwk_token.py"""
- tests/unit/authentication/test_k8s.py:1 — """Unit tests for auth/k8s module."""
- tests/unit/authentication/test_auth.py:1 — """Unit tests for functions defined in auth/init.py"""
tests/unit/authentication/test_auth.py (1)
3-4: Fix outdated module reference in file docstring.Docstring still points to
auth/__init__.py.Apply:
-"""Unit tests for functions defined in auth/__init__.py""" +"""Unit tests for functions defined in authentication/__init__.py"""tests/unit/authentication/test_noop_with_token.py (1)
6-6: Fix outdated module reference in file docstring.Docstring still points to
auth/noop_with_token.py.Apply:
-"""Unit tests for functions defined in auth/noop_with_token.py""" +"""Unit tests for functions defined in authentication/noop_with_token.py"""src/lightspeed_stack.py (1)
87-87: Polish the comment grammar.Use a clearer phrasing.
Apply:
-# if every previous steps don't fail, start the service on specified port +# If all previous steps succeed, start the service on the configured port.tests/unit/authentication/test_noop.py (1)
1-1: Fix outdated module reference in the test docstring.Keep docs in sync with the rename.
-"""Unit tests for functions defined in auth/noop.py""" +"""Unit tests for functions defined in authentication/noop.py"""src/app/endpoints/query.py (1)
319-321: Bug: wrong value returned for combined model identifier.When falling back to the first available LLM, the function returns (model_id, model_id, provider_id) instead of ("provider/model", model_id, provider_id). This can break downstream calls expecting the provider-prefixed identifier.
Apply:
- logger.info("Selected model: %s", model) - return model_id, model_id, provider_id + logger.info("Selected model: %s", model) + llama_stack_model_id = f"{provider_id}/{model_id}" + return llama_stack_model_id, model_id, provider_idtests/unit/authentication/test_k8s.py (8)
12-17: Imports updated correctly. Minor nit: update the header docstring to reflect authentication.Apply:
-"""Unit tests for auth/k8s module.""" +"""Unit tests for authentication/k8s module."""
78-80: Prefer patch.object for classmethod targets to make tests resilient to future renames.Apply:
- mock_authn_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authn_api") - mock_authz_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authz_api") + mock_authn_api = mocker.patch.object(K8sClientSingleton, "get_authn_api") + mock_authz_api = mocker.patch.object(K8sClientSingleton, "get_authz_api")
111-113: Same here: use patch.object on the class.- mock_authn_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authn_api") - mock_authz_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authz_api") + mock_authn_api = mocker.patch.object(K8sClientSingleton, "get_authn_api") + mock_authz_api = mocker.patch.object(K8sClientSingleton, "get_authz_api")
141-141: Use patch.object for classmethod.- mock_authz_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authz_api") + mock_authz_api = mocker.patch.object(K8sClientSingleton, "get_authz_api")
165-166: Use patch.object for classmethod.- mocker.patch( - "authentication.k8s.K8sClientSingleton.get_cluster_id", + mocker.patch.object( + K8sClientSingleton, + "get_cluster_id", return_value="some-cluster-id", )
195-196: Use patch.object for classmethod.- mock_get_custom_objects_api = mocker.patch( - "authentication.k8s.K8sClientSingleton.get_custom_objects_api" - ) + mock_get_custom_objects_api = mocker.patch.object( + K8sClientSingleton, "get_custom_objects_api" + )
233-237: Good path fixes; consider also patch.object for classmethod/_get_cluster_id and keep RUNNING_IN_CLUSTER as-is.- mocker.patch("authentication.k8s.RUNNING_IN_CLUSTER", True) - mocker.patch("authentication.k8s.K8sClientSingleton.__new__") - mock_get_cluster_id = mocker.patch( - "authentication.k8s.K8sClientSingleton._get_cluster_id" - ) + mocker.patch("authentication.k8s.RUNNING_IN_CLUSTER", True) + mocker.patch.object(K8sClientSingleton, "__new__") + mock_get_cluster_id = mocker.patch.object(K8sClientSingleton, "_get_cluster_id")
245-247: Same here: use patch.object for new.- mocker.patch("authentication.k8s.K8sClientSingleton.__new__") + mocker.patch.object(K8sClientSingleton, "__new__")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
pyproject.toml(1 hunks)src/app/endpoints/authorized.py(1 hunks)src/app/endpoints/config.py(1 hunks)src/app/endpoints/conversations.py(1 hunks)src/app/endpoints/feedback.py(1 hunks)src/app/endpoints/health.py(1 hunks)src/app/endpoints/info.py(1 hunks)src/app/endpoints/metrics.py(1 hunks)src/app/endpoints/models.py(1 hunks)src/app/endpoints/query.py(1 hunks)src/app/endpoints/root.py(1 hunks)src/app/endpoints/streaming_query.py(1 hunks)src/authentication/__init__.py(1 hunks)src/authentication/jwk_token.py(1 hunks)src/authentication/k8s.py(1 hunks)src/authentication/noop.py(1 hunks)src/authentication/noop_with_token.py(1 hunks)src/authorization/resolvers.py(1 hunks)src/lightspeed_stack.py(1 hunks)tests/unit/app/endpoints/test_authorized.py(1 hunks)tests/unit/authentication/test_auth.py(1 hunks)tests/unit/authentication/test_jwk_token.py(1 hunks)tests/unit/authentication/test_k8s.py(8 hunks)tests/unit/authentication/test_noop.py(1 hunks)tests/unit/authentication/test_noop_with_token.py(1 hunks)tests/unit/authentication/test_utils.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T11:09:40.404Z
Learnt from: radofuchs
PR: lightspeed-core/lightspeed-stack#485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
Applied to files:
tests/unit/authentication/test_auth.pytests/unit/authentication/test_noop.pytests/unit/authentication/test_noop_with_token.py
🧬 Code graph analysis (23)
src/authentication/k8s.py (1)
src/authentication/interface.py (1)
AuthInterface(34-39)
src/app/endpoints/conversations.py (1)
src/authentication/__init__.py (1)
get_auth_dependency(14-43)
src/authentication/noop.py (1)
src/authentication/interface.py (1)
AuthInterface(34-39)
src/app/endpoints/authorized.py (1)
src/authentication/__init__.py (1)
get_auth_dependency(14-43)
tests/unit/authentication/test_jwk_token.py (1)
src/authentication/jwk_token.py (1)
JwkTokenAuthDependency(112-194)
src/app/endpoints/info.py (1)
src/authentication/__init__.py (1)
get_auth_dependency(14-43)
src/app/endpoints/health.py (1)
src/authentication/__init__.py (1)
get_auth_dependency(14-43)
src/app/endpoints/streaming_query.py (1)
src/authentication/__init__.py (1)
get_auth_dependency(14-43)
tests/unit/authentication/test_utils.py (1)
src/authentication/utils.py (1)
extract_user_token(7-26)
tests/unit/app/endpoints/test_authorized.py (1)
src/authentication/utils.py (1)
extract_user_token(7-26)
src/authentication/noop_with_token.py (2)
src/authentication/interface.py (1)
AuthInterface(34-39)src/authentication/utils.py (1)
extract_user_token(7-26)
tests/unit/authentication/test_auth.py (1)
src/authentication/__init__.py (1)
get_auth_dependency(14-43)
src/authentication/__init__.py (1)
src/authentication/interface.py (1)
AuthInterface(34-39)
src/app/endpoints/query.py (1)
src/authentication/__init__.py (1)
get_auth_dependency(14-43)
tests/unit/authentication/test_noop.py (1)
src/authentication/noop.py (1)
NoopAuthDependency(18-43)
tests/unit/authentication/test_k8s.py (1)
src/authentication/k8s.py (2)
K8sClientSingleton(30-164)get_cluster_id(154-164)
src/app/endpoints/config.py (1)
src/authentication/__init__.py (1)
get_auth_dependency(14-43)
src/app/endpoints/root.py (1)
src/authentication/__init__.py (1)
get_auth_dependency(14-43)
src/app/endpoints/feedback.py (1)
src/authentication/__init__.py (1)
get_auth_dependency(14-43)
src/app/endpoints/metrics.py (1)
src/authentication/__init__.py (1)
get_auth_dependency(14-43)
src/app/endpoints/models.py (1)
src/authentication/__init__.py (1)
get_auth_dependency(14-43)
src/authentication/jwk_token.py (2)
src/authentication/interface.py (1)
AuthInterface(34-39)src/authentication/utils.py (1)
extract_user_token(7-26)
tests/unit/authentication/test_noop_with_token.py (1)
src/authentication/noop_with_token.py (1)
NoopWithTokenAuthDependency(27-56)
⏰ 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: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (19)
src/authorization/resolvers.py (1)
11-11: LGTM: import path updated to authentication.interface.tests/unit/app/endpoints/test_authorized.py (1)
8-8: LGTM: switched to authentication.utils.src/authentication/k8s.py (1)
14-14: LGTM: AuthInterface import path corrected.tests/unit/authentication/test_jwk_token.py (1)
12-12: LGTM: updated import to authentication.jwk_token.src/app/endpoints/conversations.py (1)
13-13: LGTM: using authentication.get_auth_dependency.tests/unit/authentication/test_utils.py (1)
6-6: LGTM: import moved to authentication.utils.src/app/endpoints/streaming_query.py (1)
23-24: Import path migration looks good.src/app/endpoints/authorized.py (1)
8-9: Import path migration looks good.src/app/endpoints/health.py (1)
15-16: Import path migration looks good.src/app/endpoints/config.py (1)
8-9: Import path migration looks good.src/app/endpoints/info.py (1)
10-11: Import path migration looks good.src/authentication/jwk_token.py (1)
21-22: Updated imports are consistent with the new package name.No functional changes introduced here.
tests/unit/authentication/test_noop.py (1)
4-4: Import path update LGTM.Tests now import from authentication.noop as intended.
src/app/endpoints/metrics.py (1)
11-12: Auth imports migrated correctly.Endpoint behavior unchanged.
src/app/endpoints/feedback.py (1)
11-12: Auth imports migrated correctly.No runtime impact.
src/app/endpoints/root.py (1)
9-10: Auth imports migrated correctly.No behavioral changes.
src/app/endpoints/query.py (1)
21-22: Auth imports migrated correctly.No behavioral changes from this edit.
src/app/endpoints/models.py (1)
10-11: Import-path migration looks correct — audit inconclusiveSwitch to authentication.* aligns with the PR intent. No behavioral impact.
- Import scan: no 'auth' Python imports found.
- Path-like scan failed (PCRE2 compile error); re-run:
rg -nS 'auth/' -g '!/node_modules/' -g '!**/*.lock' | rg -n -v 'authorization' || echo "No stale 'auth/' mentions found."- Directory check produced no output; re-run:
if ! fd -H '^auth$' src -t d >/dev/null; then echo "No src/auth directory present."; else fd -H '^auth$' src -t d; fitests/unit/authentication/test_k8s.py (1)
155-156: LGTM on target path update.
| # service/ols/src/auth/k8s.py and currently has 58 Pyright issues. It | ||
| # might need to be rewritten down the line. | ||
| "src/auth/k8s.py", | ||
| "src/authentication/k8s.py", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM: updated Pyright exclude path.
Change aligns with the module rename.
Run this to ensure no lingering auth imports/paths remain:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1/3] Python imports still using 'auth'..."
rg -nP --type=py -C2 $'^(\\s*)from\\s+auth\\b|^(\\s*)import\\s+auth(\\s|\\.|$)'
echo "[2/3] Non-import references to 'auth/<...>' (excluding authentication/authorization)..."
rg -nP -S '(?<![a-z])auth/(?!entication|orization)[^ \t"\'\)\]]+'
echo "[3/3] Package init re-exports expose get_auth_dependency..."
rg -nP --type=py -C2 '\bdef\s+get_auth_dependency\b|get_auth_dependency\s*=' src/authentication/__init__.pyLength of output: 42
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1/3] Python imports still using 'auth'..."
rg -nP --type=py -C2 "^(\\s*)from\\s+auth\\b|^(\\s*)import\\s+auth(\\s|\\.|$)" || true
echo "[2/3] Non-import references to 'auth/<...>' (excluding authentication/authorization)..."
rg -nP -S "(?<![a-z])auth/(?!entication|orization)[^ \t\"'\\)\\]]+" || true
echo "[3/3] Package init re-exports expose get_auth_dependency..."
if [ -f "src/authentication/__init__.py" ]; then
rg -nP -C2 '\bdef\s+get_auth_dependency\b|get_auth_dependency\s*=' "src/authentication/__init__.py" || true
else
echo "[3/3] src/authentication/__init__.py not found"
fiLength of output: 377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1/5] Python imports still using 'auth'..."
rg -n --type=py -C2 '^\s*(from|import)\s+auth(\b|\.)' || true
echo "[2/5] Any occurrences of 'auth.' (attribute/module usage)..."
rg -n --type=py -C2 '\bauth\.' || true
echo "[3/5] Raw 'auth/' occurrences (strings/paths) - full repo..."
rg -n -F 'auth/' -S || true
echo "[3/5] Filtered 'auth/' excluding 'authentication'/'authorization'..."
rg -n -F 'auth/' -S | rg -n -v -i 'authentication|authorization' || true
echo "[4/5] Files or directories with 'auth' in their path..."
rg --hidden --files | rg -n '/auth($|/)' || true
echo "[5/5] src/authentication/__init__.py (first 200 lines) to inspect re-exports..."
if [ -f src/authentication/__init__.py ]; then
nl -ba src/authentication/__init__.py | sed -n '1,200p'
else
echo "src/authentication/__init__.py not found"
fiLength of output: 2078
Hold — update remaining auth references to authentication
Pyright exclude change aligns with the rename, but leftover references to "auth" remain — update the following:
- pyproject.toml — comments referencing auth/k8s (lines 28, 59).
- tests/unit/authentication/* — test docstrings reference auth/*.py: tests/unit/authentication/test_noop_with_token.py, test_noop.py, test_jwk_token.py, test_auth.py, test_k8s.py, test_utils.py.
- src/authentication/noop_with_token.py — docstring references auth.utils.extract_user_token (also check noop.py and jwk_token.py docstrings).
No "import auth" / "from auth ..." occurrences were found.
🤖 Prompt for AI Agents
In pyproject.toml around lines 28 and 59 and in pyproject.toml line 61, update
any comment or path text that still says "auth" or "auth/k8s" to
"authentication" or "authentication/k8s" to match the rename; then update test
docstrings under tests/unit/authentication/* (test_noop_with_token.py,
test_noop.py, test_jwk_token.py, test_auth.py, test_k8s.py, test_utils.py) to
reference authentication/*.py instead of auth/*.py; also update docstrings in
src/authentication/noop_with_token.py (and verify noop.py and jwk_token.py) to
reference authentication.utils.extract_user_token (not
auth.utils.extract_user_token). Ensure only textual references are changed, not
imports.
| from authentication.interface import AuthInterface | ||
| from authentication import noop, noop_with_token, k8s, jwk_token |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Rename looks good; consider relative imports + optional compatibility shim.
- Relative imports reduce churn if the package name changes again.
- If any out-of-tree code still imports auth.*, consider a temporary shim package to avoid breakage.
Apply this local import tweak:
-from authentication.interface import AuthInterface
-from authentication import noop, noop_with_token, k8s, jwk_token
+from .interface import AuthInterface
+from . import noop, noop_with_token, k8s, jwk_tokenOptional: add a deprecation shim to keep old imports working during transition:
+// New file: src/auth/__init__.py
+import warnings, importlib, sys
+warnings.warn(
+ "Package 'auth' has been renamed to 'authentication'. Please update imports.",
+ DeprecationWarning, stacklevel=2
+)
+# Re-export submodules so auth.k8s, auth.interface, etc. continue to work.
+for _mod in ("k8s", "interface", "noop", "noop_with_token", "jwk_token"):
+ sys.modules[__name__ + "." + _mod] = importlib.import_module("authentication." + _mod)Verify no lingering imports from the old path:
🏁 Script executed:
#!/bin/bash
# Find any remaining 'auth.' imports/usages.
rg -nP -C2 '\bfrom\s+auth(\.| \b)|\bimport\s+auth\b|\bauth\.' --glob '!**/.venv/**' --glob '!**/node_modules/**'Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Attempting robust search for 'auth' imports/usages..."
echo "1) ripgrep (unrestricted, include hidden, ignore VCS ignore):"
rg -n -uu --hidden --no-ignore-vcs -C2 \
-e '^\s*from\s+auth\b' \
-e '^\s*import\s+auth\b' \
-e '\bauth\.' \
-g '!**/node_modules/**' -g '!**/.venv/**' || echo "No matches from ripgrep (attempt 1)"
echo "2) Fallback: search tracked files (if in a git repo):"
if git rev-parse --is-inside-work-tree >/dev/null 2>&1; then
git ls-files | while IFS= read -r f; do
rg -n -C2 -e '^\s*from\s+auth\b' -e '^\s*import\s+auth\b' -e '\bauth\.' "$f" || true
done || true
else
echo "Not a git repo; skipping fallback."
fi
echo "Search complete."Length of output: 1072
Use relative imports and add a temporary auth compatibility shim — fix remaining auth. references
Search found a legacy auth. usage in noop_with_token.py; update package-local imports or add a shim to avoid breaking consumers.
Change in src/authentication/init.py (lines 5–6):
-from authentication.interface import AuthInterface
-from authentication import noop, noop_with_token, k8s, jwk_token
+from .interface import AuthInterface
+from . import noop, noop_with_token, k8s, jwk_tokenOptional temporary shim (new file src/auth/init.py):
import warnings, importlib, sys
warnings.warn(
"Package 'auth' has been renamed to 'authentication'. Update imports.",
DeprecationWarning, stacklevel=2
)
for _mod in ("k8s", "interface", "noop", "noop_with_token", "jwk_token"):
sys.modules[__name__ + "." + _mod] = importlib.import_module("authentication." + _mod)🤖 Prompt for AI Agents
In src/authentication/__init__.py around lines 5 to 6, replace absolute package
imports with relative imports (e.g., from .interface import AuthInterface and
from . import noop, noop_with_token, k8s, jwk_token) so internal modules use
package-local imports, and add a temporary compatibility shim at
src/auth/__init__.py that emits a DeprecationWarning and exposes authentication
submodules under the old auth package by importing authentication.k8s,
authentication.interface, authentication.noop, authentication.noop_with_token,
and authentication.jwk_token into sys.modules for backward compatibility until
callers are updated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
tests/unit/authentication/test_k8s.py (6)
78-79: Patch objects directly to make tests resilient to future renames.Using the imported class avoids brittle string targets.
Apply:
- mock_authn_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authn_api") - mock_authz_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authz_api") + mock_authn_api = mocker.patch.object(K8sClientSingleton, "get_authn_api") + mock_authz_api = mocker.patch.object(K8sClientSingleton, "get_authz_api")
111-112: DRY and robustness: same patching improvement applies here.Consider a fixture for these two common patches.
- mock_authn_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authn_api") - mock_authz_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authz_api") + mock_authn_api = mocker.patch.object(K8sClientSingleton, "get_authn_api") + mock_authz_api = mocker.patch.object(K8sClientSingleton, "get_authz_api")
141-142: Same here: prefer patch.object for class attributes.- mock_authz_api = mocker.patch("authentication.k8s.K8sClientSingleton.get_authz_api") + mock_authz_api = mocker.patch.object(K8sClientSingleton, "get_authz_api")
155-155: Tighten mocks: autospec function; patch class attribute via object.Adds signature safety and avoids string targets for classmethods.
- mocker.patch( - "authentication.k8s.get_user_info", + mocker.patch( + "authentication.k8s.get_user_info", + autospec=True, return_value=MockK8sResponseStatus( authenticated=True, allowed=True, username="kube:admin", uid="some-uuid", groups=["ols-group"], ), ) - mocker.patch( - "authentication.k8s.K8sClientSingleton.get_cluster_id", - return_value="some-cluster-id", - ) + mocker.patch.object( + K8sClientSingleton, + "get_cluster_id", + return_value="some-cluster-id", + )Also applies to: 165-167
195-195: Prefer patch.object for class method here as well.- mock_get_custom_objects_api = mocker.patch( - "authentication.k8s.K8sClientSingleton.get_custom_objects_api" - ) + mock_get_custom_objects_api = mocker.patch.object( + K8sClientSingleton, "get_custom_objects_api" + )
233-237: Reset singleton state to avoid cross-test leakage.
K8sClientSingleton._get_cluster_id()in earlier tests sets_cluster_id; without resetting here, this test can pass without exercising the in‑cluster branch.Apply:
mock_get_cluster_id = mocker.patch( "authentication.k8s.K8sClientSingleton._get_cluster_id" ) mock_get_cluster_id.return_value = "some-cluster-id" + # ensure clean state so in-cluster path is exercised + K8sClientSingleton._cluster_id = None assert K8sClientSingleton.get_cluster_id() == "some-cluster-id"tests/unit/authentication/test_jwk_token.py (2)
3-3: Docstring rename looks good; tiny consistency nit.Prefer dotted module path in prose to match Python imports (authentication.jwk_token).
-"""Unit tests for functions defined in authentication/jwk_token.py""" +"""Unit tests for functions defined in authentication.jwk_token"""
12-12: Avoid importing private _jwk_cache — add and use clear_jwk_cachetests/unit/authentication/test_jwk_token.py imports the private _jwk_cache; add a small public helper in src/authentication/jwk_token.py and update the test to call it (rename the fixture to avoid shadowing).
-from authentication.jwk_token import JwkTokenAuthDependency, _jwk_cache +from authentication.jwk_token import JwkTokenAuthDependency, clear_jwk_cache-@pytest.fixture(autouse=True) -def clear_jwk_cache(): - """Clear the global JWK cache before each test.""" - _jwk_cache.clear() - yield - _jwk_cache.clear() +@pytest.fixture(autouse=True) +def reset_jwk_cache(): + """Clear the global JWK cache before each test.""" + clear_jwk_cache() + yield + clear_jwk_cache()Add to src/authentication/jwk_token.py near the module-level cache defs:
def clear_jwk_cache() -> None: """Testing utility to reset the JWK cache.""" _jwk_cache.clear()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pyproject.toml(2 hunks)src/authentication/noop_with_token.py(2 hunks)tests/unit/authentication/test_auth.py(1 hunks)tests/unit/authentication/test_jwk_token.py(2 hunks)tests/unit/authentication/test_k8s.py(9 hunks)tests/unit/authentication/test_noop.py(1 hunks)tests/unit/authentication/test_noop_with_token.py(1 hunks)tests/unit/authentication/test_utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/unit/authentication/test_noop_with_token.py
- tests/unit/authentication/test_utils.py
- pyproject.toml
- tests/unit/authentication/test_noop.py
- src/authentication/noop_with_token.py
- tests/unit/authentication/test_auth.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/authentication/test_jwk_token.py (1)
src/authentication/jwk_token.py (1)
JwkTokenAuthDependency(112-194)
tests/unit/authentication/test_k8s.py (1)
src/authentication/k8s.py (2)
K8sClientSingleton(30-164)get_cluster_id(154-164)
⏰ 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: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (3)
tests/unit/authentication/test_k8s.py (3)
1-1: Docstring rename matches module path.Consistent with the refactor. No issues.
245-246: Good isolation for out-of-cluster case.Patching
RUNNING_IN_CLUSTERand stubbing__new__is appropriate; you also reset_cluster_idbelow, which avoids state leakage.
12-17: Verified — exports present. K8sClientSingleton (L30), K8SAuthDependency (L212), ClusterIDUnavailableError (L26), and CLUSTER_ID_LOCAL (L20) are defined in src/authentication/k8s.py; no legacy auth.* references remain.
1 participant
Description
LCORE-675: rename module
authtoauthenticationto distinguish it fromauthorizationType of change
Related Tickets & Documents
Summary by CodeRabbit
No user-facing behavior changes. All authentication-related endpoints and features continue to work as before.