You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
APICAuth.get_auth(), CatalystCenterAuth.get_auth(), and SDWANManagerAuth.get_auth() all resolve ControllerContext + credentials + verify_ssl and delegate to the respective get_token/get_session_auth/get_token_auth helpers.
Grep across src/, tests/, and the consumer repos (nac-sdwan-terraform, nac-catalystcenter-terraform, ACI-as-Code-Demo) shows they're only exercised by this package's own unit tests — no production code path calls any of them.
Pre-existing pattern (predates #46), flagged in that PR's review since the file was touched by the migration.
What to do
Two clean options:
Keep them as a public API surface — document the intended external callers (or the intent) in the class docstrings, and add non-test-only coverage that reflects that use case. Otherwise it reads as ceremonial code with unit tests that only exist to keep it green.
Remove them — along with the unit-test coverage that only exists to exercise them (TestAPIC.get_auth, TestCatalystCenter.get_auth, TestSDWAN.get_auth). The migration in refactor: replace direct env var reads with nac-test core controller resolvers #46 has the test bases resolve ControllerContext/credentials themselves and pass the pieces into the specific get_token/get_session_auth/get_token_auth methods — get_auth() is a wrapper for a call pattern nobody uses.
Either is fine; the current state (dead-in-production + test-only coverage) is the worst of both worlds.
P.S. — This comment was drafted using voice-to-text via Claude Code. If the tone comes across as overly direct or terse, please know that's just how it tends to phrase things. No offense or criticism is intended — this is purely an objective technical review of the PR. Thanks for understanding! 🙂
Follow-up from #46.
Context
APICAuth.get_auth(),CatalystCenterAuth.get_auth(), andSDWANManagerAuth.get_auth()all resolveControllerContext+ credentials +verify_ssland delegate to the respectiveget_token/get_session_auth/get_token_authhelpers.Grep across
src/,tests/, and the consumer repos (nac-sdwan-terraform,nac-catalystcenter-terraform,ACI-as-Code-Demo) shows they're only exercised by this package's own unit tests — no production code path calls any of them.Pre-existing pattern (predates #46), flagged in that PR's review since the file was touched by the migration.
What to do
Two clean options:
Keep them as a public API surface — document the intended external callers (or the intent) in the class docstrings, and add non-test-only coverage that reflects that use case. Otherwise it reads as ceremonial code with unit tests that only exist to keep it green.
Remove them — along with the unit-test coverage that only exists to exercise them (
TestAPIC.get_auth,TestCatalystCenter.get_auth,TestSDWAN.get_auth). The migration in refactor: replace direct env var reads with nac-test core controller resolvers #46 has the test bases resolveControllerContext/credentials themselves and pass the pieces into the specificget_token/get_session_auth/get_token_authmethods —get_auth()is a wrapper for a call pattern nobody uses.Either is fine; the current state (dead-in-production + test-only coverage) is the worst of both worlds.
P.S. — This comment was drafted using voice-to-text via Claude Code. If the tone comes across as overly direct or terse, please know that's just how it tends to phrase things. No offense or criticism is intended — this is purely an objective technical review of the PR. Thanks for understanding! 🙂