feat(secrets): migrate Secrets service and consumers to NemoClient typed HTTP client - #609
Merged
matthewgrossman merged 10 commits intoJul 8, 2026
Conversation
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates secret access and secret CRUD from ChangesSecrets plugin client
Consumer call-site migrations
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
services/core/jobs/src/nmp/core/jobs/app/dispatcher.py (1)
220-231: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist client creation out of the loop.
client_from_platform(...)runs perenv_varin a nested loop; build it once before thefor steploop.♻️ Proposed change
+ secrets = client_from_platform(sdk_to_use, AsyncSecretsClient) for step in job_spec.steps: if not step.environment: continue for env_var in step.environment: if env_var.from_secret: workspace, secret_name = get_entity_parts(env_var.from_secret.name, default_workspace=job_workspace) - secrets = client_from_platform(sdk_to_use, AsyncSecretsClient) try: await secrets.get_secret(name=secret_name, workspace=workspace)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/core/jobs/src/nmp/core/jobs/app/dispatcher.py` around lines 220 - 231, Hoist the repeated client construction out of the nested env_var/step loop in dispatcher logic: `client_from_platform(sdk_to_use, AsyncSecretsClient)` should be created once before iterating through steps, then reused for each secret validation call. Update the secret validation flow in the surrounding dispatcher method so the existing `secrets.get_secret(...)` and exception handling continue to work with the shared client instance.services/core/secrets/tests/integration/test_secrets_with_auth.py (1)
89-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated
client_from_platform(sdk, SecretsClient)boilerplate.Consider a small fixture/helper (e.g.
secrets_client_for(user_sdk)) to reduce this ~40-occurrence pattern.Also applies to: 103-105, 117-121, 130-131, 144-148, 157-159, 176-179, 191-195, 204-209, 220-224, 233-235, 257-262, 282-290, 309-316, 334-345, 363-374, 392-400, 418-421, 444-449, 468-479, 496-503, 520-528, 542-552, 561-566, 579-595, 606-639, 651-657, 674-680, 695-701, 743-755, 776-779, 790-794, 809-818, 836-839, 850-851, 869-881, 899-915, 929-941, 963-971, 995-1002, 1016-1021, 1031-1039
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/core/secrets/tests/integration/test_secrets_with_auth.py` around lines 89 - 93, The test file repeats the same client setup pattern many times, creating noisy boilerplate around client_from_platform(..., SecretsClient). Add a small helper or pytest fixture such as secrets_client_for(user_sdk) in the shared test setup and replace the repeated SecretsClient construction in this test suite with that helper. Use the existing SecretsClient-based calls in test_secrets_with_auth as the main migration target so all create/read/update/delete secret operations go through the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/setup.py`:
- Around line 393-397: Both existence checks currently treat any client
exception as “missing”; update the helper logic in _provider_exists and
_secret_exists to catch only NotFoundError so other failures still propagate.
Import NotFoundError from nemo_platform_plugin.client.errors, and keep the
existing return True/False behavior in the provider/secret lookup paths while
narrowing the except clause to that specific symbol.
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/client/MIGRATION.md`:
- Around line 330-334: Update the migration docs entry for SecretsClient to
remove the nonexistent SecretsClient.admin reference and describe
rotate_encryption_keys as a direct method on SecretsClient. Use the
rotate_encryption_keys symbol in the table header/description so the API mapping
clearly points to the flat client method instead of an admin router.
In `@services/core/secrets/tests/integration/test_secrets_with_auth.py`:
- Around line 622-628: The assertion in
test_service_credentials_can_list_secrets is a no-op because list(...) always
returns a value, so the test does not verify list_secrets behavior. Update the
test to assert something meaningful about the result from
SecretsClient.list_secrets, such as the expected items or at least that the
returned collection is non-empty, using the existing service_sdk and
service_secrets setup.
---
Nitpick comments:
In `@services/core/jobs/src/nmp/core/jobs/app/dispatcher.py`:
- Around line 220-231: Hoist the repeated client construction out of the nested
env_var/step loop in dispatcher logic: `client_from_platform(sdk_to_use,
AsyncSecretsClient)` should be created once before iterating through steps, then
reused for each secret validation call. Update the secret validation flow in the
surrounding dispatcher method so the existing `secrets.get_secret(...)` and
exception handling continue to work with the shared client instance.
In `@services/core/secrets/tests/integration/test_secrets_with_auth.py`:
- Around line 89-93: The test file repeats the same client setup pattern many
times, creating noisy boilerplate around client_from_platform(...,
SecretsClient). Add a small helper or pytest fixture such as
secrets_client_for(user_sdk) in the shared test setup and replace the repeated
SecretsClient construction in this test suite with that helper. Use the existing
SecretsClient-based calls in test_secrets_with_auth as the main migration target
so all create/read/update/delete secret operations go through the helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3b50febf-451c-449a-acfe-eb79ff496981
⛔ Files ignored due to path filters (2)
sdk/python/nemo-platform/src/nemo_platform/cli/commands/setup.pyis excluded by!sdk/**sdk/python/nemo-platform/tests/vendored/nemo_platform_ext/cli/commands/test_setup.pyis excluded by!sdk/**
📒 Files selected for processing (21)
packages/data_designer_nemo/src/data_designer_nemo/secret_resolver.pypackages/nemo_platform_ext/src/nemo_platform_ext/cli/commands/setup.pypackages/nemo_platform_ext/tests/cli/commands/test_setup.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/client/MIGRATION.mdpackages/nemo_platform_plugin/src/nemo_platform_plugin/secrets/client.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/secrets/endpoints.pypackages/nemo_platform_plugin/src/nemo_platform_plugin/secrets/types.pypackages/nemo_platform_plugin/tests/secrets/test_client.pypackages/nemo_platform_plugin/tests/secrets/test_endpoints.pyservices/core/files/src/nmp/core/files/api/endpoint_helpers.pyservices/core/files/tests/test_endpoint_helpers.pyservices/core/inference-gateway/src/nmp/core/inference_gateway/api/proxy.pyservices/core/jobs/src/nmp/core/jobs/app/dispatcher.pyservices/core/models/src/nmp/core/models/api/permissions.pyservices/core/models/src/nmp/core/models/controllers/backends/docker/backend.pyservices/core/models/src/nmp/core/models/controllers/backends/docker/creation_reconciler.pyservices/core/secrets/src/nmp/core/secrets/api/v2/secrets/schemas.pyservices/core/secrets/tests/conftest.pyservices/core/secrets/tests/integration/test_secrets_with_auth.pyservices/core/secrets/tests/integration/test_secrets_workspace_validation.pyservices/core/secrets/tests/test_secrets_sdk.py
Contributor
|
…e-to-nemoclient-typed-http-client Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
matthewgrossman
changed the title
- Format test_secrets_sdk.py (line-length wrap ruff format flagged in CI). - Keep the shared secrets schema descriptions identical to the pre-migration OpenAPI spec so the regenerated spec stays byte-identical and the is-up-to-date / lint-openapi checks pass without a Stainless SDK round-trip (the changes were cosmetic description wording only). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-evaluator/examples/plugin_examples.py`:
- Line 249: The exception handlers in the secret lookup flow are catching the
wrong error type, so missing-secret and create-conflict cases are not handled by
the plugin client exceptions. Import NotFoundError and ConflictError from
nemo_platform_plugin.client.errors in this module, and update the
AsyncSecretsClient try/except blocks that currently catch NotFoundError so they
use those imported plugin errors in the relevant secret read/create paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f6c12b5f-a1e2-4a25-ba5f-f2aa0c7ea6b3
📒 Files selected for processing (28)
packages/nemo_platform_plugin/src/nemo_platform_plugin/client/MIGRATION.mdplugins/nemo-data-designer/src/nemo_data_designer_plugin/cli/personas.pyplugins/nemo-data-designer/src/nemo_data_designer_plugin/testing/utils.pyplugins/nemo-data-designer/tests/integration/test_personas_cli.pyplugins/nemo-evaluator/examples/plugin_examples.pyservices/core/files/tests/integration/external_storage/test_ngc_storage.pyservices/core/files/tests/integration/external_storage/test_s3_storage.pyservices/core/files/tests/integration/test_filesets_allowed_hosts.pyservices/core/files/tests/integration/tests_filesets_with_auth_secrets.pyservices/core/files/tests/test_otlp_ingest.pyservices/core/inference-gateway/src/nmp/core/inference_gateway/testing/harness.pyservices/core/inference-gateway/tests/unit/conftest.pyservices/core/jobs/tests/integration/test_jobs_secrets_access.pyservices/core/jobs/tests/integration/test_task_auth_runtime.pyservices/core/models/tests/integration/test_models_with_auth.pyservices/core/models/tests/unit/controllers/test_backend_registry.pyservices/core/models/tests/unit/controllers/test_docker_backend.pyservices/core/models/tests/unit/controllers/test_k8s_nim_operator_backend.pyservices/core/models/tests/unit/test_model_deployment_service_unit.pyservices/core/secrets/tests/test_secrets_entities.pytests/agentic-use/data-designer-config-cli-easy/tests/test_outputs.pytests/agentic-use/data-designer-config-cli/tests/test_outputs.pytests/agentic-use/inference-provider-reg-cli-easy/tests/test_outputs.pytests/agentic-use/inference-provider-reg-cli/tests/test_outputs.pytests/agentic-use/secrets-crud-cli-easy/tests/test_outputs.pytests/agentic-use/secrets-crud-cli/tests/test_outputs.pytests/agentic-use/seed_providers.pytests/agentic-use/tests/test_seed_providers.py
💤 Files with no reviewable changes (6)
- services/core/inference-gateway/tests/unit/conftest.py
- services/core/models/tests/unit/controllers/test_k8s_nim_operator_backend.py
- services/core/files/tests/test_otlp_ingest.py
- services/core/models/tests/unit/controllers/test_backend_registry.py
- services/core/models/tests/unit/test_model_deployment_service_unit.py
- services/core/models/tests/unit/controllers/test_docker_backend.py
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
maxdubrinsky
approved these changes
Jul 8, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
Jul 8, 2026
…e-to-nemoclient-typed-http-client Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> # Conflicts: # plugins/nemo-data-designer/tests/integration/test_personas_cli.py # plugins/nemo-evaluator/examples/plugin_examples.py # services/core/files/tests/integration/external_storage/test_ngc_storage.py # services/core/files/tests/integration/external_storage/test_s3_storage.py # services/core/files/tests/integration/test_filesets_allowed_hosts.py # services/core/files/tests/integration/tests_filesets_with_auth_secrets.py # services/core/jobs/src/nmp/core/jobs/app/dispatcher.py # services/core/models/src/nmp/core/models/api/permissions.py # services/core/models/tests/integration/test_models_with_auth.py
matthewgrossman
deleted the
mgrossman/aircore-873-migrate-secrets-service-to-nemoclient-typed-http-client
branch
July 8, 2026 23:12
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
…ped HTTP client (#609) * code review Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * add migration files Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * fix migration Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * fix(secrets): resolve CI lint failures - Format test_secrets_sdk.py (line-length wrap ruff format flagged in CI). - Keep the shared secrets schema descriptions identical to the pre-migration OpenAPI spec so the regenerated spec stays byte-identical and the is-up-to-date / lint-openapi checks pass without a Stainless SDK round-trip (the changes were cosmetic description wording only). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * fixe tests Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * update migration.md Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * docs: fold in Files/Jobs migration learnings, keep playbook general Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> * fix issues Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> --------- Signed-off-by: Matthew Grossman <mgrossman@nvidia.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
matthewgrossman
added a commit
that referenced
this pull request
Jul 13, 2026
…o-nemoclient-typed-http-client Brings in the merged NemoClient migrations from main: - #584 (Files consumers -> FilesClient) - #609 (Secrets service + consumers -> SecretsClient) - #614 (plugin-client URL encoding) Resolved 4 conflicts where the Jobs consumer migration (AIRCORE-874) overlapped the Files/Secrets migrations in multi-service files: - services/core/entities/.../workspace_cleanup.py: combined both migrations (jobs via AsyncJobsClient, filesets via AsyncFilesClient); switched PlatformJobStatus to the plugin type and built the terminal-status set from enum members. - services/core/entities/tests/.../test_workspace_cleanup.py: adopted #584's tuple-return _make_sdk + _MockAsyncPaginatedResponse; added a dispatching _patch_clients() so the combined _async_step test routes client_from_platform to the jobs vs files mock by client class. - plugins/nemo-safe-synthesizer/.../api/v2/jobs/endpoints.py test: dropped the now-orphaned Stainless NotFoundError/PermissionDeniedError import (both uses migrated across the two PRs); made _patch_jobs_client dispatch jobs vs files clients by class. - plugins/nemo-data-designer/.../testing/utils.py: kept both the jobs (CreatePlatformJobRequest) and secrets (SecretsClient) imports. All affected suites green; jobs service unchanged (5 pre-existing env failures). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates the Secrets service (AIRCORE-873) off the Stainless-generated
NeMoPlatformSDK onto the typedNemoClientHTTP client, following the Files/Filesets template. This is the second per-service migration under the AIRCORE-827 program, and the goal is to eliminate Stainless usage for secrets so the SDK dependency can eventually be dropped.Also adds a reusable migration playbook (
nemo_platform_plugin/client/MIGRATION.md) so the remaining core-service migrations (Models, Entities, Jobs, …) can follow the same path — it folds in learnings from this PR plus the parallel Files (#584) and Jobs (#585) migrations.What changed
New typed client + shared contract
nemo_platform_plugin/secrets/{types,endpoints,client}.py—SecretsClient/AsyncSecretsClientcovering all 7 endpoints (CRUD,access, adminrotate-encryption-keys), with request/response models as the single source of truth.api/v2/secrets/schemas.pynow re-exports those plugin types (subclassing only to keepfrom_entity), so client and server can't drift.valuefields areSecretStrwith a JSONfield_serializerso the plaintext (not"**********") reaches the wire — covered by a regression test.Every
sdk.secrets.*consumer migratedAll real Stainless secrets calls now go through the typed client via
client_from_platform(sdk, SecretsClient)at the leaf (thesdkkeeps being passed around unchanged; DI/wiring is untouched — that's the separate AIRCORE-883 effort):endpoint_helpers), jobs (dispatcher), models (permissions, dockerbackend/creation_reconciler), inference-gateway (proxy), data_designer (secret_resolver), and the data-designerpersonasCLI command.setup.pyhelpers.agentic-usee2e suites. Stalemock.secrets.*stubs in already-migrated consumers were removed (dead code once the call routes through the adapter).Error handling uses
nemo_platform_plugin.client.errors.*(aliasedClient*where a file still catches Stainless errors for its non-secrets calls).Out of scope (deliberately)
nemo secretsCLI — mostly template-generated commands; migrating them would mean creating net-new generator overrides just to swap the client. Deferred to the CLI-generator fix (AIRCORE-893).jobs/.../subprocess_runtime.py— raw HTTP that never used the SDK (goal is to stop using Stainless, not force NemoClient on everything).Test plan
packages/nemo_platform_plugin/tests/secrets/— client + endpoint testsservices/core/secrets/tests/(unit + integration incl. RBAC) — passruff check/ruff format/ty check— clean (0 net-new type errors vs. baseline)Related
endpoints.py)Summary by CodeRabbit