fix(identity-resolution): correct config environment keys - #2029
Conversation
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar> (cherry picked from commit de64d7566f8878bf1d081706c31bfcf696d0f6f4)
|
📝 WalkthroughWalkthroughIdentity-resolution environment-variable overrides now use ChangesIdentity-resolution configuration and E2E wiring
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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
🤖 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 `@charts/insight/templates/secrets.yaml`:
- Around line 317-338: Refresh documentation at
charts/insight/templates/secrets.yaml lines 317-338 to distinguish hyphenated
YAML gear names from underscored environment-variable keys; update _rust_env()
in src/ingestion/tests/e2e/lib/identity.py lines 303-309 so its docstring no
longer claims every gear segment remains hyphenated; and revise
src/backend/services/identity-resolution/README.md line 42 to remove the
obsolete requirement to use env instead of export.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a6cfe1e6-7822-4873-aa0f-cf4d25fe6194
📒 Files selected for processing (11)
charts/insight/templates/secrets.yamlcharts/insight/values.yamldeploy/gitops/scripts/compose-app-secrets.shdocker-compose.ymlsrc/backend/services/identity-resolution/README.mdsrc/backend/services/identity-resolution/config/insight.yamlsrc/backend/services/identity-resolution/helm/templates/deployment.yamlsrc/backend/services/identity-resolution/helm/values.yamlsrc/backend/services/identity-resolution/src/config.rssrc/backend/services/identity-resolution/src/gear.rssrc/ingestion/tests/e2e/lib/identity.py
Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker-compose.yml (1)
121-124: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftProvide an upgrade path for the new ClickHouse init scripts.
clickhouse-datapersists the database directory, so upgrading existing Compose installs will not pick up these new01-presentation-role.sql/02-presentation-user.sqlscripts under/docker-entrypoint-initdb.d. Analytics requires thepresentationuser, so add an idempotent upgrade migration or document a safe reset/migration step; otherwise fresh-install validation can pass while existing environments fail to connect read-only.🤖 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 `@docker-compose.yml` around lines 121 - 124, Add an upgrade path for existing persistent ClickHouse installations alongside the new `01-presentation-role.sql` and `02-presentation-user.sql` mounts: provide an idempotent migration that creates the read-only presentation role and user when absent, or document the required safe migration/reset procedure. Ensure existing `clickhouse-data` volumes receive the `presentation` user without affecting fresh initialization.
🤖 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.
Outside diff comments:
In `@docker-compose.yml`:
- Around line 121-124: Add an upgrade path for existing persistent ClickHouse
installations alongside the new `01-presentation-role.sql` and
`02-presentation-user.sql` mounts: provide an idempotent migration that creates
the read-only presentation role and user when absent, or document the required
safe migration/reset procedure. Ensure existing `clickhouse-data` volumes
receive the `presentation` user without affecting fresh initialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 43f5a4fc-bf4e-4af3-8215-3e294b8db442
📒 Files selected for processing (6)
charts/insight/templates/secrets.yamlcharts/insight/values.yamldeploy/gitops/scripts/compose-app-secrets.shdocker-compose.ymlsrc/backend/services/identity-resolution/README.mdsrc/ingestion/tests/e2e/lib/identity.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ingestion/tests/e2e/lib/identity.py
- src/backend/services/identity-resolution/README.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ingestion/tests/e2e/lib/identity.py (1)
305-310: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the underscored key for the CLI tenant override.
Line 308 still uses
identity-resolution, sorun_seed_cli(tenant=...)does not settenant_default_id. Fresh-database seed runs will therefore fail tenant resolution instead of using the supplied tenant.Proposed fix
- env["APP__gears__identity-resolution__config__tenant_default_id"] = tenant + env["APP__gears__identity_resolution__config__tenant_default_id"] = tenant🤖 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 `@src/ingestion/tests/e2e/lib/identity.py` around lines 305 - 310, Update the tenant environment override in run_seed_cli to use the underscored configuration key identity_resolution instead of identity-resolution, so the supplied tenant reaches tenant_default_id during fresh-database seed runs. Preserve the existing conditional override and extra_env merge behavior.
🤖 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.
Outside diff comments:
In `@src/ingestion/tests/e2e/lib/identity.py`:
- Around line 305-310: Update the tenant environment override in run_seed_cli to
use the underscored configuration key identity_resolution instead of
identity-resolution, so the supplied tenant reaches tenant_default_id during
fresh-database seed runs. Preserve the existing conditional override and
extra_env merge behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28178ce4-a2b8-44e0-b07b-c3c252e037c9
📒 Files selected for processing (5)
charts/insight/templates/secrets.yamlcharts/insight/values.yamlsrc/backend/services/identity-resolution/helm/values.yamlsrc/backend/services/identity-resolution/src/gear.rssrc/ingestion/tests/e2e/lib/identity.py
Normalize the identity-resolution env-key segments arriving with the seed CronJob and seed CLI to underscores. Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
Normalize the identity-resolution env-key segment on the incoming sync CronJob and its e2e rig to underscores. Signed-off-by: Aleksandr Barkhatov <pm@aleks.bar>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/services/identity-resolution/helm/tests/test_seed_cronjob_contract.py (1)
1-14: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the CronJob count assertion with the stated contract.
The module documentation says the suite must tolerate a third CronJob and select jobs by name. The test still requires exactly
len(JOBS)rendered CronJobs. A future unrelated CronJob will fail this test.Remove the total-count assertion, or restrict the count to identity-resolution CronJobs.
Proposed test adjustment
-def test_default_render_ships_exactly_the_two_documented_cronjobs(default_docs) -> None: - names = sorted(_cronjobs(default_docs)) - assert len(names) == len(JOBS), names +def test_default_render_ships_documented_cronjobs(default_docs) -> None: for job in JOBS: _cronjob(default_docs, job)Also applies to: 169-174
🤖 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 `@src/backend/services/identity-resolution/helm/tests/test_seed_cronjob_contract.py` around lines 1 - 14, Remove the exact total CronJob count assertion from the test module, since the contract must tolerate unrelated CronJobs. Keep selecting and validating the identity-resolution jobs by their names through the existing JOBS-based logic, preserving all per-job assertions.
🧹 Nitpick comments (1)
src/backend/services/identity-resolution/src/gear.rs (1)
110-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep rustdoc only on shared-library APIs.
This code is in the identity-resolution service module. If the crate is a binary or service crate, remove the new
///block. Keep it only ifCargo.tomldefines this module as part of a shared library.As per coding guidelines: use
///documentation comments only on exported items in shared library crates; do not add them to binaries or services.🤖 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 `@src/backend/services/identity-resolution/src/gear.rs` around lines 110 - 117, Remove the added rustdoc block describing the sync subcommand unless Cargo.toml confirms this module belongs to a shared-library crate; for the identity-resolution service binary, keep the subcommand implementation unchanged and omit the `///` documentation comments.Source: Coding guidelines
🤖 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.
Outside diff comments:
In
`@src/backend/services/identity-resolution/helm/tests/test_seed_cronjob_contract.py`:
- Around line 1-14: Remove the exact total CronJob count assertion from the test
module, since the contract must tolerate unrelated CronJobs. Keep selecting and
validating the identity-resolution jobs by their names through the existing
JOBS-based logic, preserving all per-job assertions.
---
Nitpick comments:
In `@src/backend/services/identity-resolution/src/gear.rs`:
- Around line 110-117: Remove the added rustdoc block describing the sync
subcommand unless Cargo.toml confirms this module belongs to a shared-library
crate; for the identity-resolution service binary, keep the subcommand
implementation unchanged and omit the `///` documentation comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d9ebddff-818b-44d1-9352-62b106804885
📒 Files selected for processing (11)
charts/insight/templates/secrets.yamlcharts/insight/values.yamldeploy/gitops/scripts/compose-app-secrets.shdocker-compose.ymlsrc/backend/services/identity-resolution/README.mdsrc/backend/services/identity-resolution/helm/templates/seed-cronjob.yamlsrc/backend/services/identity-resolution/helm/templates/sync-cronjob.yamlsrc/backend/services/identity-resolution/helm/tests/test_seed_cronjob_contract.pysrc/backend/services/identity-resolution/helm/values.yamlsrc/backend/services/identity-resolution/seed/smoke-orgchart-visibility.shsrc/backend/services/identity-resolution/src/gear.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/backend/services/identity-resolution/README.md
- charts/insight/templates/secrets.yaml
4 participants
Summary
Validation
docker compose --env-file .env.compose.example -f docker-compose.yml config --quietbash -n deploy/gitops/scripts/compose-app-secrets.shSummary by CodeRabbit
identity_resolutionnamespace across deployments, secrets, database settings, and tenant configuration.