[danielmawhirter]

Summary

Google integration tests require explicit local ADC files instead of retrieving keys from AWS, decoding environment keys, or discovering home-directory credentials. Typed BigQuery identities select restricted key files or impersonation targets; workload mode requires federated ADC.

Credential setup follows test skip selection and precedes resource fixtures. The base identity survives worker teardown, each test restores credential environment and GCS client cache, and input files remain unchanged. Mixed local invocations that already started DiskGraph workers fail with instructions to use a separate invocation. Depends on #224; production connector authentication semantics and existing check names are preserved.

Change map

Change map

Validation

Signed head 9920279c includes main's merged #174 Rust action fix through #224.

• 84 combined local credential/workflow tests pass; changed-file flake8/isort and diff checks pass. Documentation checker: 257 files pass.
• GPU CI passes.
• Testing and Coverage passes, including every pytest shard, package suites, native checks, and aggregate coverage.
• All three GCP integration jobs pass: 10 tests across GCS, BigQuery + S3, and BigQuery ingestion.
• Connector/config: 1915 passed, 41 skipped, 1 xfailed. Both restricted BigQuery permission tests executed and passed.
• Real Snowflake filesystem invocation with invalid encoded GCP credentials: 104 tests skip without credential resolution.

The Snowflake connector job still fails one assertion because metadata includes static shared HM_SAMPLED.TEST_TABLE. This identical failure predates these PRs on main. Its creation/cleanup ownership has not been established; leftover state and permission drift remain hypotheses. The assertion is retained pending a supported fixture/contract fix.

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

BigQuery credential setup now validates an explicit local ADC file, resolves the requested identity, and scopes credential environment changes to a context. GCS setup reuses that context. Pytest hooks manage session and per-test credential contexts and reject GCP test setup after workers start. Tests and integration prerequisites now use the explicit ADC and identity model.

Sequence Diagram(s)

sequenceDiagram
  participant Test
  participant setup_bigquery_credentials
  participant resolve_credentials
  participant credential_kind
  participant Environment
  Test->>setup_bigquery_credentials: enter credential context
  setup_bigquery_credentials->>resolve_credentials: resolve selected identity
  resolve_credentials->>credential_kind: validate ADC credential file
  credential_kind-->>resolve_credentials: return credential kind
  resolve_credentials-->>setup_bigquery_credentials: return ADC path and impersonation target
  setup_bigquery_credentials->>Environment: apply scoped credential values
  Test->>setup_bigquery_credentials: exit credential context
  setup_bigquery_credentials->>Environment: restore prior values

Loading

Merge Risk: Minimal · up to c9a24

No actionable merge-blocking code risk is established. The PR change-table validation failure still needs diagnosis.

Architecture risk: Low · up to c9a24

The change affects 2 systems.

Changed systems: test, kumo

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — test (service) was modified; 8 changed files map to changed impact.
• observed — kumo (service) was modified; 2 changed files map to changed impact.

Before / after behavior

• observed — Modified behavior in kumo/testing/bigquery.py: Imports and credential configuration declarations are replaced: secret retrieval, logging, legacy key identifiers, and key-to-impersonation mapping are removed. New environment-variable names, a pytest stash key, credential environment-variable list, and BigQueryIdentity and CredentialKind enums define explicit identity and credential types.
• observed — Modified behavior in kumo/testing/bigquery.py: credential_kind validates that the path is local, exists, and is not a directory, then parses a JSON object and maps its type to CredentialKind. Invalid JSON structure, unsupported types, or invalid paths raise RuntimeError.
• observed — Modified behavior in kumo/testing/bigquery.py: resolve_credentials requires a BigQueryIdentity and an explicit ADC path, validates its credential type, and requires federated ADC when workload exchange is enabled. DEFAULT uses that ADC directly. For NO_TABLE and NO_JOB, service-account-key ADC requires the corresponding restricted credential-file environment variable containing another key; other credential types require the matching impersonation target and return the original ADC path with that target.
• observed — Modified behavior in kumo/testing/bigquery.py: setup_bigquery_credentials changes from configuring global environment and retrieving/writing secret-backed keys when ADC setup fails to a context manager that resolves credentials and temporarily applies ADC, impersonation, bucket, and project settings. It restores the environment on context exit; credential input files remain read-only.

Pre-merge checks: 2 · 1

Failed checks: 1 inconclusive

Check name	Status	Explanation	Resolution
Change Map Design Review		The marked change map and linked problem are not available for review. The supplied summary does not show the comment's rationale, verification, or stopping condition. The diff has 834 changed lines a…	Provide the full comment starting with <!-- kumo-pr-change-map --> and the linked problem or release requirement. Review the comment against the diff and existing behavior.

Passed checks: 2 passed

Check name	Status	Explanation
Pr Size		The pull request changes 10 files. This is below the limit of 40 files.
Public Resource Access		The changed lines add no applicable external resource references. The added S3 and GCS URLs are test inputs for rejected remote credentials.

Full details: Change Map Design Review

Explanation

The marked change map and linked problem are not available for review. The supplied summary does not show the comment's rationale, verification, or stopping condition. The diff has 834 changed lines and replaces credential fallback with scoped explicit credentials, but code alone cannot establish what the comment explains.

Resolution

Provide the full comment starting with <!-- kumo-pr-change-map --> and the linked problem or release requirement. Review the comment against the diff and existing behavior.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

Recent review info

Run configuration

• Source mode: Auto
• Repository: NVIDIA/kumo-engine
• Branch: dm-gcp-credential-cleanup
• File: .coderabbit.yaml
• Commit: c9a246b
• Layers: Repository YAML
• Review profile: QUIET
• Plan: Enterprise
• Run ID: a4a85f4f-73b8-45dc-ba1e-67ad66410b30

Commits

Reviewing files that changed from the base of the PR and between 9920279 and c9a246b.

Files selected for processing: 1

• test/rest/integ/test_bigquery_ingestion_diskgraph_e2e.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

[danielmawhirter]

Change map

File	Function / scope	What changed	Why
kumo/testing/bigquery.py	ADC resolver, typed identities, scoped setup, decorator	Require explicit local ADC, validate credential type, scope environment, remove AWS fallback and runtime wrappers.	Make selected identities deterministic and fail before resources start.
kumo/testing/gcs.py	Credential setup and decorator	Remove encoded/home lookup and writes; use explicit scoped ADC and a fresh temporary client cache.	Prevent hidden identity selection and stale cached credentials.
test/conftest.py	Credential setup and teardown hooks	Lazily scope the base session identity and each selected test after skip selection; restore after fixture teardown; reject late worker reuse.	Preserve worker credential lifetime and keep skipped tests credential-free.
test/connector/test_bigquery_connector.py	Restricted permission tests	Replace secret-name arguments with typed no-table and no-job identities.	Preserve negative tests without binding identity selection to AWS secret names.
test/io_utils/fsutil/test_all.py	GCS parameter	Mark the GCS parameter without changing node IDs.	Dynamic fixture lookup otherwise hides its credential prerequisite from setup hooks.
test/rest/integ/conftest.py	Pool subprocess creation	Record actual pool worker startup in session state.	Detect mixed invocations that would reuse workers started before GCP configuration.
test/testing/test_bigquery_credentials.py	Credential regressions	Cover supported ADC, typed restricted inputs, missing/invalid/local-only files, federation enforcement, restoration and immutable inputs.	Verify explicit configuration and eliminate fallback-only tests.
test/testing/test_gcs_credentials.py	Cache and real pytest lifecycle regressions	Verify collection performs no authentication, cache restoration, skip/setup/test/teardown ordering, late-worker rejection and real mixed-parameter collection.	Protect credential lifetime across resource failures and provider selection.
test/rest/integ/test_bigquery_diskgraph_e2e.py	Module prerequisites	Replace obsolete AWS secret instructions with explicit local ADC and workload credential requirements.	Keep the training test setup instructions consistent with the removed fallback.
test/rest/integ/test_bigquery_ingestion_diskgraph_e2e.py	Module prerequisites	Replace obsolete AWS/encoded fallback instructions with explicit ADC requirements.	Keep ingestion test setup instructions consistent with the credential policy.

Requested requirement: consolidate explicit Google test credentials and remove automatic stored-key fallback after minimal NVIDIA CI enablement. Production authentication and broad provider-environment isolation are outside this change.

[coderabbitai]

Nitpick comments: 1

test/testing/test_bigquery_credentials.py: 1

173-185: Trivial · Maintainability & Code Quality · Quick win

Use an independent mechanism to isolate the environment in test_environment_restored.

The test writes 'changed-by-test' directly into os.environ for every name in _CREDENTIAL_ENV_VARS. It relies on setup_bigquery_credentials to undo those writes. The outer monkeypatch only snapshots GOOGLE_APPLICATION_CREDENTIALS, GCP_PROJECT_ID and ENV_GOOGLE_CREDENTIALS_JSON.

If the restore logic regresses, the assertion fails. The values for GOOGLE_IMPERSONATE_SERVICE_ACCOUNT and the bucket variable then stay in the process. Later tests in the same session inherit them.

Give the test its own safety net. Replace os.environ with a copy for the duration of the test. Compute before after that replacement.

♻️ Proposed change

 def test_environment_restored(monkeypatch, explicit_adc, failure):
     names = testing_bigquery._CREDENTIAL_ENV_VARS
+    # Independent isolation: a restore bug must not leak into later tests.
+    monkeypatch.setattr(os, 'environ', os.environ.copy())
     monkeypatch.setenv('GCP_PROJECT_ID', 'previous-project')

Verify that the code under test and monkeypatch.setenv both work with a plain-dict os.environ. Both use item access only.

Based on learnings: "do not use that same function (or its mechanism) to set up, tear down, or safeguard the test."

Source: Learnings

Review info

Run configuration

• Source mode: Auto
• Repository: NVIDIA/kumo-engine
• Branch: dm-gcp-credential-cleanup
• File: .coderabbit.yaml
• Commit: 9920279
• Layers: Repository YAML
• Review profile: QUIET
• Plan: Enterprise
• Run ID: 648ae060-353b-4110-b515-a76f82f4bd83

Commits

Reviewing files that changed from the base of the PR and between 386c680 and 9920279.

Files selected for processing: 11

• docs/development/testing.md
• kumo/testing/bigquery.py
• kumo/testing/gcs.py
• test/conftest.py
• test/connector/test_bigquery_connector.py
• test/io_utils/fsutil/test_all.py
• test/rest/integ/conftest.py
• test/rest/integ/test_bigquery_diskgraph_e2e.py
• test/rest/integ/test_bigquery_ingestion_diskgraph_e2e.py
• test/testing/test_bigquery_credentials.py
• test/testing/test_gcs_credentials.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.