feat(seed): run the demo seeder on Kubernetes stands - #2253
Merged
Conversation
📝 WalkthroughWalkthroughThe seeder moved from ChangesPackaged seeder and safeguards
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
ktursunov
marked this pull request as draft
August 6, 2026 03:35
ktursunov
marked this pull request as ready for review
August 6, 2026 05:51
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
src/ingestion/tools/seed/insight_seed/config.py (1)
126-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPort parsing escapes the
EnvContractErrorcontract.Every other parser in this module reports a bad value as
EnvContractError.int(env.get("MARIADB_PORT", "3306"))andint(env.get("CLICKHOUSE_HTTP_PORT", "8123"))raise a bareValueErrorinstead.
preflight.checkcatches onlyEnvContractError, and it callsparse_mariadbat Line 344 andparse_clickhouseat Line 358 — after the aggregation gate at Line 333. A malformed port therefore produces an unhandled traceback rather than the collected problem list this module promises.♻️ Proposed fix: parse ports through a typed helper
+def _parse_port(env: Mapping[str, str], name: str, default: str) -> int: + raw = (env.get(name) or "").strip() or default + try: + return int(raw) + except ValueError as exc: + raise EnvContractError((f"{name}={raw!r} is not a port number: {exc}.",)) from exc + + def parse_mariadb(env: Mapping[str, str], *, database: str) -> MariaDb: return MariaDb( host=env.get("MARIADB_HOST", "mariadb"), - port=int(env.get("MARIADB_PORT", "3306")), + port=_parse_port(env, "MARIADB_PORT", "3306"), user=env.get("MARIADB_USER", "insight"), password=env.get("MARIADB_PASSWORD", "insight-local"), database=database, ) def parse_clickhouse(env: Mapping[str, str]) -> ClickHouse: return ClickHouse( host=env.get("CLICKHOUSE_HOST", "clickhouse"), - http_port=int(env.get("CLICKHOUSE_HTTP_PORT", "8123")), + http_port=_parse_port(env, "CLICKHOUSE_HTTP_PORT", "8123"), user=env.get("CLICKHOUSE_USER", "insight"), password=env.get("CLICKHOUSE_PASSWORD", "insight-local"), database=env.get("CLICKHOUSE_DATABASE", "insight"), )🤖 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/tools/seed/insight_seed/config.py` around lines 126 - 143, Update parse_mariadb and parse_clickhouse to parse their port environment values through the module’s existing typed validation helper, so malformed MARIADB_PORT and CLICKHOUSE_HTTP_PORT values raise EnvContractError rather than ValueError. Preserve the current defaults and resulting integer fields, and ensure preflight.check can continue collecting these errors through its existing EnvContractError handling.src/ingestion/tools/seed/tests/test_preflight.py (2)
46-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the failing case to the repeated-case assertions.
The coding guidelines ask for
pytest.mark.parametrizeon copy-pasted test cases and for the failing case in the assertion message, for examplef"should reject: {value!r}". This suite runs underunittest(see the run command at Line 9), sosubTestis the correct substitute forparametrize. The failing-case message is still missing.
subTestreports the parameters, butassertFalseat Lines 78-80 gives no value in its own message. Passmsg=so a failure names the input directly.♻️ Proposed fix
def test_the_cross_tenant_fixture_is_on_unless_turned_off(self) -> None: self.assertTrue(config.cross_tenant_fixture_enabled({})) for value in ("0", "false", "NO", "off"): with self.subTest(value=value): self.assertFalse( - config.cross_tenant_fixture_enabled({config.CROSS_TENANT_FIXTURE_ENV: value}) + config.cross_tenant_fixture_enabled({config.CROSS_TENANT_FIXTURE_ENV: value}), + msg=f"should read as off: {value!r}", )Apply the same
msg=to the blank-tenant loop at Lines 46-49.Also applies to: 76-80
🤖 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/tools/seed/tests/test_preflight.py` around lines 46 - 49, Add a value-specific msg= argument to the assertion inside test_a_blank_tenant_is_the_same_as_a_missing_one, using the current loop value so failures identify the rejected input directly. Apply the same message pattern to the repeated-case assertion around the corresponding blank-tenant validation at lines 76-80, without changing the subTest structure or validation behavior.Source: Coding guidelines
256-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the conditional expression evaluated for its side effect.
Lines 263-265 evaluate
config.parse_anchor_date(env) if config.ANCHOR_ENV in env else config.parse_seed_days(env)as a statement. The reader cannot tell which parser each case exercises without re-deriving the branch.The construct is also fragile. If a future case sets both
ANCHOR_ENVandDAYS_ENV, only the anchor parser runs and the day case is never tested. Split the cases by parser instead.♻️ Proposed fix
def test_a_malformed_window_is_refused_before_anything_is_written(self) -> None: - for env in ( - {config.ANCHOR_ENV: "30-06-2026"}, - {config.DAYS_ENV: "x"}, - {config.DAYS_ENV: "0"}, - ): - with self.subTest(env=env), self.assertRaises(config.EnvContractError): - config.parse_anchor_date( - env - ) if config.ANCHOR_ENV in env else config.parse_seed_days(env) + cases = ( + (config.parse_anchor_date, {config.ANCHOR_ENV: "30-06-2026"}), + (config.parse_seed_days, {config.DAYS_ENV: "x"}), + (config.parse_seed_days, {config.DAYS_ENV: "0"}), + ) + for parser, env in cases: + with self.subTest(parser=parser.__name__, env=env): + with self.assertRaises(config.EnvContractError, msg=f"should reject: {env!r}"): + parser(env)🤖 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/tools/seed/tests/test_preflight.py` around lines 256 - 265, Refactor test_a_malformed_window_is_refused_before_anything_is_written to separate anchor-date cases from seed-days cases, and invoke config.parse_anchor_date and config.parse_seed_days directly in their respective loops or assertions. Remove the conditional expression used only for its side effect, ensuring each malformed environment is explicitly tested against the intended parser, including cases where both variables may be present.src/ingestion/tools/seed/insight_seed/preflight.py (2)
171-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the ClickHouse client instead of
object.
_tenant_columnsand_foreign_silver_rowstakeclient: objectand then suppress the resulting attribute error with# type: ignore[attr-defined]at Lines 176 and 224. The suppression hides any future signature drift inquery.Define a small structural protocol. The type checker then verifies the two call sites, and
_FakeClickHouseintest_preflight.pysatisfies it without a cast.♻️ Proposed protocol
+class _QueryClient(Protocol): + def query(self, sql: str, parameters: dict[str, object] | None = ...) -> Any: ... + + -def _tenant_columns(client: object) -> dict[tuple[str, str], str]: +def _tenant_columns(client: _QueryClient) -> dict[tuple[str, str], str]: """Which column carries the tenant, per reset target that has one.""" from .generators.base import RESET_TARGETS schemas = sorted({schema for schema, _ in RESET_TARGETS}) - found = client.query( # type: ignore[attr-defined] + found = client.query(Apply the same change to
_foreign_silver_rowsat Line 195 and to the call at Line 224.🤖 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/tools/seed/insight_seed/preflight.py` around lines 171 - 196, Define a small structural protocol for the ClickHouse client with the query interface used by the preflight helpers, then change the client parameters in _tenant_columns and _foreign_silver_rows to that protocol. Remove the attr-defined type-ignore comments at both query call sites and ensure the protocol accepts the existing query arguments and result shape so _FakeClickHouse satisfies it without casts.
232-253: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClose the ClickHouse client.
_check_clickhousecreates aclickhouse_connect.Clientand does not release it. After the initialSELECT 1, wrap the foreign-silver-rows check and missing-script checks soclient.close()runs infinallywhen a client was created.🤖 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/tools/seed/insight_seed/preflight.py` around lines 232 - 253, The _check_clickhouse function must always close a successfully created client. Wrap the existing foreign-silver-rows and missing-script checks in a finally block guarded by client is not None, and call client.close() there while preserving the current problem collection and return behavior.src/ingestion/tools/seed/insight_seed/identity.py (2)
143-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSplit this module before it grows further.
src/ingestion/tools/seed/insight_seed/identity.pynow reaches line 498. Extract the observation writers or orchestration code into separate modules.As per coding guidelines,
src/ingestion/**/*.pymodules must stay below approximately 400 lines.🤖 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/tools/seed/insight_seed/identity.py` around lines 143 - 175, The identity.py module exceeds the approximately 400-line limit; split its observation-writer or orchestration responsibilities into separate modules. Move cohesive functions such as seed_persons and their directly related helpers/constants together, update imports and call sites, and preserve the existing seeding behavior and public interfaces.Source: Coding guidelines
38-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce explanatory comments to the required scope.
Keep only one-line comments that state an invariant or workaround. Move detailed operational explanation into named helpers, tests, or external documentation.
src/ingestion/tools/seed/insight_seed/identity.py#L38-L48: reduce the seed-reason explanation to one invariant comment.src/ingestion/tools/seed/insight_seed/identity.py#L77-L128: reduce the SQL and deduplication explanation to concise invariant comments.src/ingestion/tools/seed/insight_seed/keycloak_realm.py#L3-L17: remove the module header that restates package ownership and usage.src/ingestion/tools/seed/insight_seed/silver.py#L141-L145: reduce the date-window explanation to one comment.As per coding guidelines, comments must state reasons that code cannot express and must stay to one line.
🤖 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/tools/seed/insight_seed/identity.py` around lines 38 - 48, Reduce comments to one-line invariant or workaround statements: in identity.py lines 38-48 keep only the seed-reason ownership invariant; in identity.py lines 77-128 replace SQL and deduplication explanations with concise invariant comments; in keycloak_realm.py lines 3-17 remove the module header; and in silver.py lines 141-145 condense the date-window explanation to one comment. Preserve the existing behavior and move no explanatory detail into new inline comments.Source: Coding guidelines
src/ingestion/tools/seed/insight_seed/keycloak_realm.py (1)
73-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace exposed
Anyreturn types.Define JSON value aliases or
TypedDictmodels for these realm documents.dict[str, Any]lets untyped values escape from every changed helper.As per coding guidelines,
src/ingestion/**/*.{py,pyi}must not allow bareAnyto escape.Also applies to: 152-152, 170-172, 192-192, 222-227
🤖 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/tools/seed/insight_seed/keycloak_realm.py` at line 73, Replace the exposed Any-based return annotations in _protocol_mappers and the other changed realm-document helpers with project-compatible JSON value aliases or precise TypedDict models. Update all referenced helper signatures and their returned structures so no dict[str, Any] or bare Any escapes from these functions, while preserving the existing realm document shapes and behavior.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.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 493: Update the seed-module references to include the insight_seed
package directory: CONTRIBUTING.md:493 and .env.compose.example:178 should
reference profiles.py; tests/stand/README.md:74 should reference
golden_metrics.py; tests/stand/api/identity/test_internal.py:53 and :64 should
reference profiles.py; and tests/stand/conftest.py:217 should reference
analytics.py, all under src/ingestion/tools/seed/insight_seed/.
In `@docker-compose.yml`:
- Line 386: Update all relocated seed-module references: docker-compose.yml
lines 386 and 729 to insight_seed/identity.py and insight_seed/profiles.py
respectively; dev-compose.sh lines 491, 691-695, and 1542 to
insight_seed/profiles.py, insight_seed/profiles.py::get_login_id_pairs, and
insight_seed/generators/ respectively; tests/generate_schemas.py line 27 to
insight_seed/render_profile.py; tests/lib/insight_stand/manifest.py lines 191
and 263 to insight_seed/golden_metrics.py and insight_seed/analytics.py; and
tests/lib/insight_stand/personas.py line 73 to
insight_seed/profiles.py::build_other_tenant_roster.
In `@src/ingestion/tools/seed/insight_seed/identity.py`:
- Around line 159-174: Make the full identity seed operation atomic by acquiring
a single MariaDB advisory lock before any observation existence checks begin,
and release it only after both the roster seeding loop and seed_login_ids
complete. Update the enclosing identity seed function and ensure all callers
follow the locked path, preventing concurrent processes from interleaving
_observation_exists checks and inserts.
In `@src/ingestion/tools/seed/insight_seed/keycloak_realm.py`:
- Around line 285-292: Replace the fallback-based tenant_id lookup in the realm
generation flow with config.parse_tenant_id(os.environ). Remove the hardcoded
tenant ID so generation requires and validates TENANT_DEFAULT_ID, keeping realm
claims aligned with the seeded tenant.
In `@src/ingestion/tools/seed/insight_seed/preflight.py`:
- Around line 257-264: Update the exception handling around _foreign_silver_rows
in the preflight check so only the expected fresh ClickHouse-database case is
treated as an empty scan. For all other failures, append a preflight refusal to
problems and include the exception context or SEED_FORCE guidance, preventing
execution from proceeding to TRUNCATE; preserve the existing zero-row fallback
only for the fresh-database case.
In `@src/ingestion/tools/seed/README.md`:
- Line 88: Update the README statement near the seed.py reference to remove the
trailing space from the inline code span, placing the separating space in
regular text instead so the code span contains only seed.py.
In `@src/ingestion/tools/seed/seed-stand.sh`:
- Line 139: Update the --deadline parsing in seed-stand.sh to validate
DEADLINE_SECONDS before discovery or Job rendering, accepting only numeric
values greater than zero. Reject non-numeric and non-positive inputs with the
script’s argument-error path, preventing invalid values from reaching
activeDeadlineSeconds or later SECONDS arithmetic.
In `@src/ingestion/tools/seed/tests/__init__.py`:
- Line 1: Remove the module-level docstring from tests/__init__.py, leaving the
file empty because no package initialization is required.
In `@src/ingestion/tools/seed/tests/test_identity.py`:
- Around line 26-28: Update the test fixture’s setUp() and tearDown() methods to
save and restore both IDP_SOURCE_TYPE and AUTH_MODE, setting IDP_SOURCE_TYPE to
fakeidp for each test instead of using setdefault() at module scope. Remove the
global environment mutation so tests cannot inherit state from one another or
the caller’s shell.
---
Nitpick comments:
In `@src/ingestion/tools/seed/insight_seed/config.py`:
- Around line 126-143: Update parse_mariadb and parse_clickhouse to parse their
port environment values through the module’s existing typed validation helper,
so malformed MARIADB_PORT and CLICKHOUSE_HTTP_PORT values raise EnvContractError
rather than ValueError. Preserve the current defaults and resulting integer
fields, and ensure preflight.check can continue collecting these errors through
its existing EnvContractError handling.
In `@src/ingestion/tools/seed/insight_seed/identity.py`:
- Around line 143-175: The identity.py module exceeds the approximately 400-line
limit; split its observation-writer or orchestration responsibilities into
separate modules. Move cohesive functions such as seed_persons and their
directly related helpers/constants together, update imports and call sites, and
preserve the existing seeding behavior and public interfaces.
- Around line 38-48: Reduce comments to one-line invariant or workaround
statements: in identity.py lines 38-48 keep only the seed-reason ownership
invariant; in identity.py lines 77-128 replace SQL and deduplication
explanations with concise invariant comments; in keycloak_realm.py lines 3-17
remove the module header; and in silver.py lines 141-145 condense the
date-window explanation to one comment. Preserve the existing behavior and move
no explanatory detail into new inline comments.
In `@src/ingestion/tools/seed/insight_seed/keycloak_realm.py`:
- Line 73: Replace the exposed Any-based return annotations in _protocol_mappers
and the other changed realm-document helpers with project-compatible JSON value
aliases or precise TypedDict models. Update all referenced helper signatures and
their returned structures so no dict[str, Any] or bare Any escapes from these
functions, while preserving the existing realm document shapes and behavior.
In `@src/ingestion/tools/seed/insight_seed/preflight.py`:
- Around line 171-196: Define a small structural protocol for the ClickHouse
client with the query interface used by the preflight helpers, then change the
client parameters in _tenant_columns and _foreign_silver_rows to that protocol.
Remove the attr-defined type-ignore comments at both query call sites and ensure
the protocol accepts the existing query arguments and result shape so
_FakeClickHouse satisfies it without casts.
- Around line 232-253: The _check_clickhouse function must always close a
successfully created client. Wrap the existing foreign-silver-rows and
missing-script checks in a finally block guarded by client is not None, and call
client.close() there while preserving the current problem collection and return
behavior.
In `@src/ingestion/tools/seed/tests/test_preflight.py`:
- Around line 46-49: Add a value-specific msg= argument to the assertion inside
test_a_blank_tenant_is_the_same_as_a_missing_one, using the current loop value
so failures identify the rejected input directly. Apply the same message pattern
to the repeated-case assertion around the corresponding blank-tenant validation
at lines 76-80, without changing the subTest structure or validation behavior.
- Around line 256-265: Refactor
test_a_malformed_window_is_refused_before_anything_is_written to separate
anchor-date cases from seed-days cases, and invoke config.parse_anchor_date and
config.parse_seed_days directly in their respective loops or assertions. Remove
the conditional expression used only for its side effect, ensuring each
malformed environment is explicitly tested against the intended parser,
including cases where both variables may be present.
🪄 Autofix
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: 2c1ee0f4-e192-46f2-b6ba-dd2c92d620e5
📥 Commits
Reviewing files that changed from the base of the PR and between db9ba22 and f6b08ca4adc5a14b7215e50f9a29d37e388a4d67.
📒 Files selected for processing (60)
.cf-studio/config/README.md.cf-studio/version.toml.env.compose.example.github/dependabot.yml.github/workflows/e2e-stand.yml.gitignoreCONTRIBUTING.mddeploy/HELM_DEPLOY.mddeploy/compose/insight-init.shdeploy/compose/keycloak/README.mddeploy/compose/ui-tests.Dockerfiledeploy/seed/README.mddeploy/seed/pyproject.tomldeploy/seed/seed.pydev-compose.shdocker-compose.ymldocs/TESTING.mdsrc/ingestion/.dockerignoresrc/ingestion/tools/seed/Dockerfilesrc/ingestion/tools/seed/PROFILE.mdsrc/ingestion/tools/seed/README.mdsrc/ingestion/tools/seed/insight_seed/__init__.pysrc/ingestion/tools/seed/insight_seed/__main__.pysrc/ingestion/tools/seed/insight_seed/analytics.pysrc/ingestion/tools/seed/insight_seed/config.pysrc/ingestion/tools/seed/insight_seed/generators/__init__.pysrc/ingestion/tools/seed/insight_seed/generators/ai.pysrc/ingestion/tools/seed/insight_seed/generators/base.pysrc/ingestion/tools/seed/insight_seed/generators/collab.pysrc/ingestion/tools/seed/insight_seed/generators/crm.pysrc/ingestion/tools/seed/insight_seed/generators/git.pysrc/ingestion/tools/seed/insight_seed/generators/hr.pysrc/ingestion/tools/seed/insight_seed/generators/people.pysrc/ingestion/tools/seed/insight_seed/generators/support.pysrc/ingestion/tools/seed/insight_seed/generators/task.pysrc/ingestion/tools/seed/insight_seed/golden_metrics.pysrc/ingestion/tools/seed/insight_seed/identity.pysrc/ingestion/tools/seed/insight_seed/keycloak_realm.pysrc/ingestion/tools/seed/insight_seed/manifest.pysrc/ingestion/tools/seed/insight_seed/preflight.pysrc/ingestion/tools/seed/insight_seed/profile_md.pysrc/ingestion/tools/seed/insight_seed/profiles.pysrc/ingestion/tools/seed/insight_seed/render_profile.pysrc/ingestion/tools/seed/insight_seed/silver.pysrc/ingestion/tools/seed/pyproject.tomlsrc/ingestion/tools/seed/seed-job.yaml.tplsrc/ingestion/tools/seed/seed-stand.shsrc/ingestion/tools/seed/tests/__init__.pysrc/ingestion/tools/seed/tests/test_identity.pysrc/ingestion/tools/seed/tests/test_preflight.pysrc/ingestion/tools/toolbox/Dockerfiletests/generate_schemas.pytests/lib/insight_stand/__init__.pytests/lib/insight_stand/manifest.pytests/lib/insight_stand/personas.pytests/pyproject.tomltests/stand/README.mdtests/stand/api/identity/test_internal.pytests/stand/conftest.pytests/versions.yaml
💤 Files with no reviewable changes (3)
- deploy/seed/README.md
- deploy/seed/seed.py
- deploy/seed/pyproject.toml
Comment on lines
+159
to
+174
| for p in roster: | ||
| if _observation_exists(cur, tenant_uuid, p.uuid, "email", "value_id", p.email): | ||
| continue | ||
| cur.execute( | ||
| sql, | ||
| ( | ||
| DEV_SEED_SOURCE_TYPE, | ||
| _bin(DEV_SEED_SOURCE_ID), | ||
| _bin(tenant_uuid), | ||
| p.email, | ||
| _bin(p.uuid), | ||
| _bin(AUTHOR_PERSON_UUID), | ||
| _REASON_ROSTER, | ||
| ), | ||
| ) | ||
| for p in roster | ||
| ] | ||
| cur.executemany(sql, rows) | ||
| return cur.rowcount | ||
| inserted += cur.rowcount |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make the identity seed operation atomic.
Two seed processes can both pass _observation_exists before either process inserts a row. The documented unique key includes created_at, so both inserts create duplicate logical observations.
Acquire one MariaDB advisory lock for the full identity seed, including seed_login_ids, before the existence checks begin.
Also applies to: 285-300
🤖 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/tools/seed/insight_seed/identity.py` around lines 159 - 174,
Make the full identity seed operation atomic by acquiring a single MariaDB
advisory lock before any observation existence checks begin, and release it only
after both the roster seeding loop and seed_login_ids complete. Update the
enclosing identity seed function and ensure all callers follow the locked path,
preventing concurrent processes from interleaving _observation_exists checks and
inserts.
ktursunov
enabled auto-merge
August 6, 2026 06:08
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/ingestion/tools/seed/insight_seed/preflight.py (1)
289-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep rationale comments to one line.
These comments explain valid cross-function invariants, but both exceed the repository limit.
src/ingestion/tools/seed/insight_seed/preflight.py#L289-L293: replace the paragraph with one concise reason for refusing a failed scan.src/ingestion/tools/seed/insight_seed/keycloak_realm.py#L286-L289: replace the paragraph with one concise reason for requiring the shared tenant parser.As per coding guidelines, “Add comments only when code cannot express the reason ...; keep them to one line.”
🤖 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/tools/seed/insight_seed/preflight.py` around lines 289 - 293, Shorten the rationale comment in src/ingestion/tools/seed/insight_seed/preflight.py#L289-L293 to one line explaining that a failed scan must block truncation. Also shorten the rationale comment in src/ingestion/tools/seed/insight_seed/keycloak_realm.py#L286-L289 to one line explaining why the shared tenant parser is required; make no code changes.Source: Coding guidelines
src/ingestion/tools/seed/Dockerfile (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the Seed Dockerfile’s published-image dependencies.
FROM python:3.12-slim(line 19) andpip install --no-cache-dir dbt-clickhouse(line 36) re-resolve on each build. Use a digest-pinned Python image and installdbt-clickhousefrom a pinned, locked version rather than an unbounded extra.🤖 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/tools/seed/Dockerfile` at line 19, Update the Seed Dockerfile’s FROM instruction to use a digest-pinned Python 3.12 slim image, and change the pip install command for dbt-clickhouse to use the project’s pinned locked version instead of an unbounded package specification. Preserve the existing installation behavior while ensuring both published-image dependencies resolve reproducibly.
🤖 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.
Nitpick comments:
In `@src/ingestion/tools/seed/Dockerfile`:
- Line 19: Update the Seed Dockerfile’s FROM instruction to use a digest-pinned
Python 3.12 slim image, and change the pip install command for dbt-clickhouse to
use the project’s pinned locked version instead of an unbounded package
specification. Preserve the existing installation behavior while ensuring both
published-image dependencies resolve reproducibly.
In `@src/ingestion/tools/seed/insight_seed/preflight.py`:
- Around line 289-293: Shorten the rationale comment in
src/ingestion/tools/seed/insight_seed/preflight.py#L289-L293 to one line
explaining that a failed scan must block truncation. Also shorten the rationale
comment in src/ingestion/tools/seed/insight_seed/keycloak_realm.py#L286-L289 to
one line explaining why the shared tenant parser is required; make no code
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: edce4627-63ec-47aa-a71e-9098068fcf1c
📥 Commits
Reviewing files that changed from the base of the PR and between f6b08ca4adc5a14b7215e50f9a29d37e388a4d67 and 4ba617d022d8f408652929d0e41b422603c68a90.
📒 Files selected for processing (23)
.github/workflows/build-images.yml.github/workflows/trivy-images.ymlCONTRIBUTING.mdREADME.mdcharts/insight/values.yamldeploy/HELM_DEPLOY.mddeploy/gitops/Makefiledeploy/gitops/environments/local/values.yaml.templatedocker-compose.ymlsrc/ingestion/README.mdsrc/ingestion/tools/seed/Dockerfilesrc/ingestion/tools/seed/PROFILE.mdsrc/ingestion/tools/seed/README.mdsrc/ingestion/tools/seed/insight_seed/config.pysrc/ingestion/tools/seed/insight_seed/keycloak_realm.pysrc/ingestion/tools/seed/insight_seed/preflight.pysrc/ingestion/tools/seed/seed-job.yaml.tplsrc/ingestion/tools/seed/seed-stand.shsrc/ingestion/tools/seed/tests/__init__.pysrc/ingestion/tools/seed/tests/test_identity.pysrc/ingestion/tools/seed/tests/test_preflight.pysrc/ingestion/tools/toolbox/Dockerfilesrc/ingestion/tools/toolbox/Dockerfile.dockerignore
🚧 Files skipped from review as they are similar to previous changes (8)
- deploy/HELM_DEPLOY.md
- docker-compose.yml
- src/ingestion/tools/seed/PROFILE.md
- src/ingestion/tools/seed/seed-job.yaml.tpl
- src/ingestion/tools/seed/insight_seed/config.py
- src/ingestion/tools/seed/seed-stand.sh
- src/ingestion/tools/seed/tests/test_preflight.py
- CONTRIBUTING.md
cyberantonz
approved these changes
Aug 6, 2026
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
.github/workflows/build-images.yml (1)
1139-1139: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSet
persist-credentials: falseon this checkout.The seed build job does not push with git. The persisted token stays in
.git/configwhile a docker build runs from the same context. zizmor flags it asartipacked.♻️ Proposed change
- - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 + with: + persist-credentials: false🤖 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 @.github/workflows/build-images.yml at line 1139, Update the checkout step in the seed build job to set persist-credentials to false, ensuring the GitHub token is not retained in .git/config during the Docker build; leave the existing actions/checkout version unchanged.Source: Linters/SAST tools
src/ingestion/tools/seed/insight_seed/config.py (1)
129-146: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRoute port parsing through
EnvContractError.Every other parser in this module raises
EnvContractErrorwith an operator-readable problem.int(env.get("MARIADB_PORT", "3306"))andint(env.get("CLICKHOUSE_HTTP_PORT", "8123"))raise a bareValueError. A stand whose ConfigMap carries a non-numeric port then aborts with a traceback that names neither the variable nor the fix, and the aggregated problem list never forms.♻️ Proposed refactor
+def _parse_port(env: Mapping[str, str], name: str, *, default: str) -> int: + raw = (env.get(name) or "").strip() or default + try: + return int(raw) + except ValueError as exc: + raise EnvContractError((f"{name}={raw!r} is not a port number.",)) from exc + + def parse_mariadb(env: Mapping[str, str], *, database: str) -> MariaDb: return MariaDb( host=env.get("MARIADB_HOST", "mariadb"), - port=int(env.get("MARIADB_PORT", "3306")), + port=_parse_port(env, "MARIADB_PORT", default="3306"), user=env.get("MARIADB_USER", "insight"), password=env.get("MARIADB_PASSWORD", "insight-local"), database=database, ) def parse_clickhouse(env: Mapping[str, str]) -> ClickHouse: return ClickHouse( host=env.get("CLICKHOUSE_HOST", "clickhouse"), - http_port=int(env.get("CLICKHOUSE_HTTP_PORT", "8123")), + http_port=_parse_port(env, "CLICKHOUSE_HTTP_PORT", default="8123"),As per coding guidelines: "Use named helpers for repetition, log detailed context at the failure site, and raise one typed exception per failure kind."
🤖 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/tools/seed/insight_seed/config.py` around lines 129 - 146, Update parse_mariadb and parse_clickhouse to parse their port environment variables through the module’s existing EnvContractError-based validation helper instead of calling int directly. Ensure invalid MARIADB_PORT and CLICKHOUSE_HTTP_PORT errors identify the variable and expected numeric value, preserving the aggregated configuration-error flow.Source: Coding guidelines
src/ingestion/tools/seed/tests/test_preflight.py (2)
260-269: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the conditional expression used as a statement.
The body evaluates a ternary and discards its value. Select the parser explicitly instead. The result reads as one clause per case and states which variable each case exercises.
♻️ Proposed refactor
def test_a_malformed_window_is_refused_before_anything_is_written(self) -> None: - for env in ( - {config.ANCHOR_ENV: "30-06-2026"}, - {config.DAYS_ENV: "x"}, - {config.DAYS_ENV: "0"}, - ): - with self.subTest(env=env), self.assertRaises(config.EnvContractError): - config.parse_anchor_date( - env - ) if config.ANCHOR_ENV in env else config.parse_seed_days(env) + cases = ( + (config.parse_anchor_date, {config.ANCHOR_ENV: "30-06-2026"}), + (config.parse_seed_days, {config.DAYS_ENV: "x"}), + (config.parse_seed_days, {config.DAYS_ENV: "0"}), + ) + for parse, env in cases: + with self.subTest(env=env), self.assertRaises(config.EnvContractError): + parse(env)As per coding guidelines: "Name intermediate values instead of nesting calls three levels deep."
🤖 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/tools/seed/tests/test_preflight.py` around lines 260 - 269, In test_a_malformed_window_is_refused_before_anything_is_written, replace the discarded conditional expression with explicit branching that calls parse_anchor_date for anchor-date cases and parse_seed_days for seed-day cases. Keep the existing subTest and EnvContractError assertions unchanged, and make each branch clearly identify the environment variable it exercises.Source: Coding guidelines
306-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the behavior, not the source text of
preflight.py.
test_the_silver_step_also_consults_the_persons_signalreadspreflight.pyas text and requires the exact substring"identity" in requested or "silver" in requested. The behavioral rule is "the silver step consults thepersonssignal". Any reformat that keeps that rule true breaks the test: a Ruff line wrap, a swap of the two operands, or a rename ofrequested. Drive the guard withrequested={"silver"}instead and assert that thepersonscount is queried.Note that
test_every_registered_target_is_actually_truncated_by_a_generatorat lines 155-166 has the same shape. It regex-matches generator source, so atruncate(client, schema, table)call written with variables or across two lines silently drops out ofcalledand the equality assertion then fails for the wrong reason.As per coding guidelines: "Name tests after the behavioral rule, not implementation mechanics."
🤖 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/tools/seed/tests/test_preflight.py` around lines 306 - 309, Rewrite test_the_silver_step_also_consults_the_persons_signal to exercise preflight behavior with requested={"silver"} and assert that the persons count is queried, rather than reading preflight.py source text. Also update test_every_registered_target_is_actually_truncated_by_a_generator to verify truncation behavior directly instead of regex-matching generator source, and rename tests as needed to describe the behavioral rules rather than implementation mechanics.Source: Coding guidelines
src/backend/services/authenticator/tests/run-e2e.sh (1)
63-63: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEmit the same tenant UUID in both the realm generation and Keycloak container envs.
KC_TENANT_IDis a new e2e-only variable, but its comment says it is the compose stack value. Use the same expression asTENANT_DEFAULT_IDso Keycloak, the generated realm users, and the seeded sample data all reference the same tenant ID if the value changes.🤖 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/authenticator/tests/run-e2e.sh` at line 63, Update KC_TENANT_ID to reuse the same tenant UUID expression as TENANT_DEFAULT_ID, ensuring the Keycloak container environment, generated realm users, and seeded sample data share one configurable tenant identifier.
🤖 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 @.github/workflows/build-images.yml:
- Around line 1213-1250: Update the merge-seed job to run the same image CVE
gate as merge-toolbox before the “Create multi-arch manifest and push” step. Add
the required checkout and invoke ./.github/actions/image-cve-gate against the
downloaded seed digests before any seed tag is published, reusing the existing
gate configuration and workflow pattern.
In `@src/ingestion/tools/seed/seed-job.yaml.tpl`:
- Around line 49-69: Add pod-level and container-level securityContext settings
to the seed Job template, using restricted-compatible defaults: run as non-root,
disable privilege escalation, drop all capabilities, and set the container root
filesystem to read-only. Mount an emptyDir volume named scratch at /tmp and
declare the matching pod volume, preserving the existing command, args, and
environment paths.
---
Nitpick comments:
In @.github/workflows/build-images.yml:
- Line 1139: Update the checkout step in the seed build job to set
persist-credentials to false, ensuring the GitHub token is not retained in
.git/config during the Docker build; leave the existing actions/checkout version
unchanged.
In `@src/backend/services/authenticator/tests/run-e2e.sh`:
- Line 63: Update KC_TENANT_ID to reuse the same tenant UUID expression as
TENANT_DEFAULT_ID, ensuring the Keycloak container environment, generated realm
users, and seeded sample data share one configurable tenant identifier.
In `@src/ingestion/tools/seed/insight_seed/config.py`:
- Around line 129-146: Update parse_mariadb and parse_clickhouse to parse their
port environment variables through the module’s existing EnvContractError-based
validation helper instead of calling int directly. Ensure invalid MARIADB_PORT
and CLICKHOUSE_HTTP_PORT errors identify the variable and expected numeric
value, preserving the aggregated configuration-error flow.
In `@src/ingestion/tools/seed/tests/test_preflight.py`:
- Around line 260-269: In
test_a_malformed_window_is_refused_before_anything_is_written, replace the
discarded conditional expression with explicit branching that calls
parse_anchor_date for anchor-date cases and parse_seed_days for seed-day cases.
Keep the existing subTest and EnvContractError assertions unchanged, and make
each branch clearly identify the environment variable it exercises.
- Around line 306-309: Rewrite
test_the_silver_step_also_consults_the_persons_signal to exercise preflight
behavior with requested={"silver"} and assert that the persons count is queried,
rather than reading preflight.py source text. Also update
test_every_registered_target_is_actually_truncated_by_a_generator to verify
truncation behavior directly instead of regex-matching generator source, and
rename tests as needed to describe the behavioral rules rather than
implementation mechanics.
🪄 Autofix
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: 20490ea0-8f2d-404e-8663-e8fa13654d92
📥 Commits
Reviewing files that changed from the base of the PR and between 2424b30 and fd4d085dd9aaffdd0ce1dcc0a48299763b2a28a2.
📒 Files selected for processing (72)
.env.compose.example.github/dependabot.yml.github/workflows/authenticator.yml.github/workflows/build-images.yml.github/workflows/e2e-stand.yml.github/workflows/trivy-images.yml.gitignoreCONTRIBUTING.mdREADME.mdcharts/insight/values.yamldeploy/HELM_DEPLOY.mddeploy/compose/insight-init.shdeploy/compose/keycloak/README.mddeploy/compose/ui-tests.Dockerfiledeploy/gitops/Makefiledeploy/gitops/environments/local/values.yaml.templatedeploy/seed/Dockerfiledeploy/seed/README.mddeploy/seed/pyproject.tomldeploy/seed/seed.pydeploy/seed/test_identity.pydev-compose.shdocker-compose.ymldocs/TESTING.mdsrc/backend/services/authenticator/tests/kc-realm-overlay.pysrc/backend/services/authenticator/tests/run-e2e.shsrc/ingestion/.dockerignoresrc/ingestion/README.mdsrc/ingestion/tools/seed/Dockerfilesrc/ingestion/tools/seed/PROFILE.mdsrc/ingestion/tools/seed/README.mdsrc/ingestion/tools/seed/insight_seed/__init__.pysrc/ingestion/tools/seed/insight_seed/__main__.pysrc/ingestion/tools/seed/insight_seed/analytics.pysrc/ingestion/tools/seed/insight_seed/config.pysrc/ingestion/tools/seed/insight_seed/generators/__init__.pysrc/ingestion/tools/seed/insight_seed/generators/ai.pysrc/ingestion/tools/seed/insight_seed/generators/base.pysrc/ingestion/tools/seed/insight_seed/generators/collab.pysrc/ingestion/tools/seed/insight_seed/generators/crm.pysrc/ingestion/tools/seed/insight_seed/generators/git.pysrc/ingestion/tools/seed/insight_seed/generators/hr.pysrc/ingestion/tools/seed/insight_seed/generators/people.pysrc/ingestion/tools/seed/insight_seed/generators/support.pysrc/ingestion/tools/seed/insight_seed/generators/task.pysrc/ingestion/tools/seed/insight_seed/golden_metrics.pysrc/ingestion/tools/seed/insight_seed/identity.pysrc/ingestion/tools/seed/insight_seed/keycloak_realm.pysrc/ingestion/tools/seed/insight_seed/manifest.pysrc/ingestion/tools/seed/insight_seed/preflight.pysrc/ingestion/tools/seed/insight_seed/profile_md.pysrc/ingestion/tools/seed/insight_seed/profiles.pysrc/ingestion/tools/seed/insight_seed/render_profile.pysrc/ingestion/tools/seed/insight_seed/silver.pysrc/ingestion/tools/seed/pyproject.tomlsrc/ingestion/tools/seed/seed-job.yaml.tplsrc/ingestion/tools/seed/seed-stand.shsrc/ingestion/tools/seed/tests/__init__.pysrc/ingestion/tools/seed/tests/test_identity.pysrc/ingestion/tools/seed/tests/test_preflight.pysrc/ingestion/tools/toolbox/Dockerfilesrc/ingestion/tools/toolbox/Dockerfile.dockerignoretests/generate_schemas.pytests/lib/insight_stand/__init__.pytests/lib/insight_stand/manifest.pytests/lib/insight_stand/personas.pytests/pyproject.tomltests/stand/README.mdtests/stand/api/identity/test_internal.pytests/stand/conftest.pytests/stand/ui/pages/group_dialog.pytests/versions.yaml
💤 Files with no reviewable changes (5)
- deploy/seed/Dockerfile
- deploy/seed/pyproject.toml
- deploy/seed/README.md
- deploy/seed/test_identity.py
- deploy/seed/seed.py
🚧 Files skipped from review as they are similar to previous changes (53)
- tests/generate_schemas.py
- src/ingestion/tools/seed/insight_seed/generators/ai.py
- src/ingestion/tools/seed/insight_seed/init.py
- src/ingestion/tools/seed/insight_seed/generators/people.py
- src/ingestion/tools/seed/insight_seed/generators/init.py
- docs/TESTING.md
- src/ingestion/tools/seed/insight_seed/profiles.py
- deploy/compose/insight-init.sh
- src/ingestion/tools/seed/insight_seed/main.py
- charts/insight/values.yaml
- src/ingestion/.dockerignore
- tests/stand/api/identity/test_internal.py
- tests/pyproject.toml
- src/ingestion/tools/seed/pyproject.toml
- src/ingestion/tools/seed/insight_seed/golden_metrics.py
- src/ingestion/tools/seed/PROFILE.md
- src/ingestion/tools/seed/insight_seed/generators/git.py
- src/ingestion/tools/seed/insight_seed/generators/hr.py
- src/ingestion/tools/toolbox/Dockerfile
- deploy/HELM_DEPLOY.md
- tests/lib/insight_stand/personas.py
- .github/workflows/trivy-images.yml
- src/ingestion/README.md
- tests/stand/conftest.py
- deploy/compose/ui-tests.Dockerfile
- src/ingestion/tools/seed/insight_seed/analytics.py
- src/ingestion/tools/seed/insight_seed/generators/crm.py
- deploy/compose/keycloak/README.md
- src/ingestion/tools/seed/insight_seed/manifest.py
- tests/lib/insight_stand/init.py
- src/ingestion/tools/seed/insight_seed/generators/collab.py
- src/ingestion/tools/seed/insight_seed/profile_md.py
- .github/workflows/e2e-stand.yml
- src/ingestion/tools/seed/Dockerfile
- README.md
- .gitignore
- tests/stand/README.md
- src/ingestion/tools/seed/insight_seed/generators/support.py
- tests/versions.yaml
- src/ingestion/tools/seed/insight_seed/silver.py
- dev-compose.sh
- src/ingestion/tools/seed/tests/test_identity.py
- tests/lib/insight_stand/manifest.py
- .github/dependabot.yml
- deploy/gitops/environments/local/values.yaml.template
- src/ingestion/tools/seed/insight_seed/render_profile.py
- src/ingestion/tools/seed/insight_seed/preflight.py
- .env.compose.example
- src/ingestion/tools/seed/insight_seed/identity.py
- src/ingestion/tools/seed/insight_seed/generators/task.py
- src/ingestion/tools/seed/insight_seed/keycloak_realm.py
- src/ingestion/tools/seed/insight_seed/generators/base.py
- docker-compose.yml
Comment on lines
+1213
to
+1250
| merge-seed: | ||
| needs: [changes, seed] | ||
| if: | | ||
| always() | ||
| && needs.seed.result != 'skipped' | ||
| && needs.changes.outputs.should_push == 'true' | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Require the platform builds to have passed | ||
| if: needs.seed.result != 'success' | ||
| run: | | ||
| echo "::error::seed finished '${{ needs.seed.result }}' — nothing to merge." | ||
| exit 1 | ||
| - name: Download digests | ||
| uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 | ||
| with: | ||
| path: /tmp/digests | ||
| pattern: digests-seed-* | ||
| merge-multiple: true | ||
| - uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v3 | ||
| - uses: docker/login-action@c94ce9fb468520275223c153574b00df6fe4bcc9 # v3 | ||
| with: | ||
| registry: ${{ env.REGISTRY }} | ||
| username: ${{ github.actor }} | ||
| password: ${{ secrets.GITHUB_TOKEN }} | ||
| - uses: docker/metadata-action@c299e40c65443455700f0fdfc63efafe5b349051 # v5 | ||
| id: meta | ||
| with: | ||
| images: ${{ env.IMAGE_PREFIX }}/insight-seed | ||
| tags: | | ||
| type=raw,value=${{ needs.changes.outputs.build_tag }} | ||
| type=raw,value=latest,enable=${{ github.ref == 'refs/heads/main' }} | ||
| - name: Create multi-arch manifest and push | ||
| working-directory: /tmp/digests | ||
| run: | | ||
| docker buildx imagetools create \ | ||
| $(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON") \ | ||
| $(printf '${{ env.IMAGE_PREFIX }}/insight-seed@sha256:%s ' *) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add the image CVE gate before the seed manifest is tagged.
merge-toolbox runs ./.github/actions/image-cve-gate before docker buildx imagetools create (lines 1083-1088), so no tag points at an image with a fixable critical. merge-seed omits both the checkout and that gate. The published insight-seed tags therefore bypass the scan that every other merged image passes.
🛡️ Proposed fix
- uses: docker/metadata-action@c299e40c65443455700f0fdfc63efafe5b349051 # v5
id: meta
with:
images: ${{ env.IMAGE_PREFIX }}/insight-seed
tags: |
type=raw,value=${{ needs.changes.outputs.build_tag }}
type=raw,value=latest,enable=${{ github.ref == 'refs/heads/main' }}
+ - uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1
+ with:
+ persist-credentials: false
+ - name: Fail on a fixable critical before any tag points here
+ uses: ./.github/actions/image-cve-gate
+ with:
+ image: ${{ env.IMAGE_PREFIX }}/insight-seed
+ registry-username: ${{ github.actor }}
+ registry-password: ${{ secrets.GITHUB_TOKEN }}
- name: Create multi-arch manifest and push📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| merge-seed: | |
| needs: [changes, seed] | |
| if: | | |
| always() | |
| && needs.seed.result != 'skipped' | |
| && needs.changes.outputs.should_push == 'true' | |
| runs-on: ubuntu-latest | |
| steps: | |
| - name: Require the platform builds to have passed | |
| if: needs.seed.result != 'success' | |
| run: | | |
| echo "::error::seed finished '${{ needs.seed.result }}' — nothing to merge." | |
| exit 1 | |
| - name: Download digests | |
| uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 | |
| with: | |
| path: /tmp/digests | |
| pattern: digests-seed-* | |
| merge-multiple: true | |
| - uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v3 | |
| - uses: docker/login-action@c94ce9fb468520275223c153574b00df6fe4bcc9 # v3 | |
| with: | |
| registry: ${{ env.REGISTRY }} | |
| username: ${{ github.actor }} | |
| password: ${{ secrets.GITHUB_TOKEN }} | |
| - uses: docker/metadata-action@c299e40c65443455700f0fdfc63efafe5b349051 # v5 | |
| id: meta | |
| with: | |
| images: ${{ env.IMAGE_PREFIX }}/insight-seed | |
| tags: | | |
| type=raw,value=${{ needs.changes.outputs.build_tag }} | |
| type=raw,value=latest,enable=${{ github.ref == 'refs/heads/main' }} | |
| - name: Create multi-arch manifest and push | |
| working-directory: /tmp/digests | |
| run: | | |
| docker buildx imagetools create \ | |
| $(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON") \ | |
| $(printf '${{ env.IMAGE_PREFIX }}/insight-seed@sha256:%s ' *) | |
| merge-seed: | |
| needs: [changes, seed] | |
| if: | | |
| always() | |
| && needs.seed.result != 'skipped' | |
| && needs.changes.outputs.should_push == 'true' | |
| runs-on: ubuntu-latest | |
| steps: | |
| - name: Require the platform builds to have passed | |
| if: needs.seed.result != 'success' | |
| run: | | |
| echo "::error::seed finished '${{ needs.seed.result }}' — nothing to merge." | |
| exit 1 | |
| - name: Download digests | |
| uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 | |
| with: | |
| path: /tmp/digests | |
| pattern: digests-seed-* | |
| merge-multiple: true | |
| - uses: docker/setup-buildx-action@8d2750c68a42422c14e847fe6c8ac0403b4cbd6f # v3 | |
| - uses: docker/login-action@c94ce9fb468520275223c153574b00df6fe4bcc9 # v3 | |
| with: | |
| registry: ${{ env.REGISTRY }} | |
| username: ${{ github.actor }} | |
| password: ${{ secrets.GITHUB_TOKEN }} | |
| - uses: docker/metadata-action@c299e40c65443455700f0fdfc63efafe5b349051 # v5 | |
| id: meta | |
| with: | |
| images: ${{ env.IMAGE_PREFIX }}/insight-seed | |
| tags: | | |
| type=raw,value=${{ needs.changes.outputs.build_tag }} | |
| type=raw,value=latest,enable=${{ github.ref == 'refs/heads/main' }} | |
| - uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1 | |
| with: | |
| persist-credentials: false | |
| - name: Fail on a fixable critical before any tag points here | |
| uses: ./.github/actions/image-cve-gate | |
| with: | |
| image: ${{ env.IMAGE_PREFIX }}/insight-seed | |
| registry-username: ${{ github.actor }} | |
| registry-password: ${{ secrets.GITHUB_TOKEN }} | |
| - name: Create multi-arch manifest and push | |
| working-directory: /tmp/digests | |
| run: | | |
| docker buildx imagetools create \ | |
| $(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON") \ | |
| $(printf '${{ env.IMAGE_PREFIX }}/insight-seed@sha256:%s ' *) |
🤖 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 @.github/workflows/build-images.yml around lines 1213 - 1250, Update the
merge-seed job to run the same image CVE gate as merge-toolbox before the
“Create multi-arch manifest and push” step. Add the required checkout and invoke
./.github/actions/image-cve-gate against the downloaded seed digests before any
seed tag is published, reusing the existing gate configuration and workflow
pattern.
Comment on lines
+49
to
+69
| spec: | ||
| restartPolicy: Never | ||
| # Suppress the legacy `<SVC>_SERVICE_HOST/PORT` env vars kubelet injects | ||
| # for every Service in the namespace — several collide by name with the | ||
| # seeder's own variables. | ||
| enableServiceLinks: false | ||
| # The same secrets the release's own Jobs use for this image; `[]` on a | ||
| # stand that pulls it anonymously. Without them a private image leaves | ||
| # the pod in ImagePullBackOff. | ||
| imagePullSecrets: ${SEED_PULL_SECRETS} | ||
| containers: | ||
| - name: seed | ||
| image: ${SEED_IMAGE} | ||
| # IfNotPresent so a locally built image can be tried on a local | ||
| # cluster without pushing it to a registry first. | ||
| imagePullPolicy: IfNotPresent | ||
| # The package is installed in the image, so this is a program on PATH | ||
| # — no shell, no working directory, nothing that depends on where the | ||
| # source happens to sit. | ||
| command: [insight-seed] | ||
| args: [${SEED_STEP}] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add a pod and container securityContext.
The pod spec sets no securityContext. The container therefore runs with the image's default user, a writable root filesystem, all default capabilities, and privilege escalation allowed. Two consequences follow. A namespace that enforces the restricted Pod Security Standard rejects this Job outright, and the --dry-run output is the reference manifest operators copy. The env block already points every writable path at /tmp, so a read-only root filesystem with a /tmp emptyDir is compatible with the current design.
🛡️ Proposed addition
spec:
restartPolicy: Never
+ securityContext:
+ runAsNonRoot: true
+ seccompProfile:
+ type: RuntimeDefault
# Suppress the legacy `<SVC>_SERVICE_HOST/PORT` env vars kubelet injects
@@
- name: seed
image: ${SEED_IMAGE}
+ securityContext:
+ allowPrivilegeEscalation: false
+ readOnlyRootFilesystem: true
+ capabilities:
+ drop: [ALL]A read-only root filesystem needs a writable /tmp:
volumeMounts:
- name: scratch
mountPath: /tmp
volumes:
- name: scratch
emptyDir: {}🧰 Tools
🪛 smarty-lint (0.3.3)
[warn] 58-58: "SEED_PULL_SECRETS" (function) should be written in lower-case.
(lower-case-identifier)
[warn] 61-61: "SEED_IMAGE" (function) should be written in lower-case.
(lower-case-identifier)
[warn] 69-69: "SEED_STEP" (function) should be written in lower-case.
(lower-case-identifier)
🤖 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/tools/seed/seed-job.yaml.tpl` around lines 49 - 69, Add
pod-level and container-level securityContext settings to the seed Job template,
using restricted-compatible defaults: run as non-root, disable privilege
escalation, drop all capabilities, and set the container root filesystem to
read-only. Mount an emptyDir volume named scratch at /tmp and declare the
matching pod volume, preserving the existing command, args, and environment
paths.
Contributor
Author
|
| value | what it is |
|---|---|
263435318d21b8e681c14492fe198d362a7d2c83 |
docker/build-push-action commit pin |
c299e40c65443455700f0fdfc63efafe5b349051 |
docker/metadata-action commit pin |
A 40-character hex string is also the shape of a legacy PAT, which is what the detector matches. These are action pins, published in every workflow that uses those actions.
They are not new here. main carries 9 and 8 occurrences of these exact SHAs in the same file today. This PR adds three more, in the seed and merge-seed jobs, pinning the same actions at the same versions as the toolbox / merge-toolbox pair they mirror. Scanning the file in isolation, main's copy yields 5 detector hits and this branch's yields 4 — the file is no worse than the one already on main. The diff scan only reads added lines, so main's copies are never re-examined and the new ones are.
Why it went red only after the last rebase. Identical content passed this check on the three earlier pushes. Those scans recorded Misses: 0, VerificationTimeSpentMS: 0 over ~190 KB; the failing ones record Misses: 2 over ~420 KB. The merge base changed what the scan covers — roughly double the bytes — and the larger scan reaches the pins. Re-running the job reproduced the failure exactly (verified_secrets: 0, unverified_secrets: 2 both times), so it is deterministic, not flaky.
No credential is involved, so there is nothing to revoke or rotate. Suppressing it means editing .github/workflows/trufflehog.yml, whose header warns against widening the filter — repo-wide scan policy, and out of scope for this PR. Raising it separately.
Every other check is green: 32 pass, 26 skip, including the required Run E2E suite.
added 10 commits
August 7, 2026 09:48
The seeder was compose-only, so a chart-deployed stand starts empty: every login is refused and every dashboard reads "No data". Make populating one a first-class path. Relocate the package to src/ingestion/tools/seed. The toolbox image's build context is already src/ingestion and its CI paths-filter is src/ingestion/**, so the published image now carries the seeder with no build-context, workflow or .dockerignore change, and a seeder edit rebuilds that image on its own. The tree splits into the `insight_seed` package and its `tests/`, with one entry point — `python3 -m insight_seed <step>` — shared by the compose image, the cluster Job and a local run. Give it an environment contract that fails loudly. TENANT_DEFAULT_ID and MARIADB_ANALYTICS_DB lose their defaults: rows written under the wrong tenant are invisible to every login while the run still reports success, and which database holds the catalogue tables is a per-stand fact — the compose stack keeps them in a database of their own, a chart-deployed stand in `mariadb.database`. A preflight module answers every such question before anything is written and reports the whole list at once instead of the first failure. Refuse rather than damage. The identity step is additive and marks every row it writes with a `reason` in its own namespace, so a tenant already holding other rows is refused. The silver step is NOT additive — it TRUNCATEs every table it writes, across all tenants — so a stand holding another tenant's rows anywhere in that reset surface is refused too. The surface is a single registry that `truncate` itself enforces, kept in step with the generator call sites by a test. SEED_FORCE=1 overrides either refusal, deliberately. Add seed-stand.sh, which reads a stand's coordinates from its own ConfigMap and Secrets, renders seed-job.yaml.tpl into a one-shot Job and follows it. Credentials never pass through the shell — the Job references the release's own Secret by key and runs as the application MariaDB user, which the chart already grants everything the seed writes. It is a plain Job rather than a chart hook, so a failed seed never gates or rolls back a release, and --dry-run prints the manifest instead of applying it. Along the way: gate the second-tenant fixture, which aborts identity-resolution's scheduled projection on a cluster and is only wanted on compose; read the activity window from one place so the manifest cannot report a window the rows do not sit in; make identity re-runs idempotent (seed_persons and seed_person_names still relied on an INSERT IGNORE collision that migration 004 removed, so each run appended duplicate email and name observations); and keep host build artefacts out of the toolbox image. Two things the tool cannot do for you, both documented: create the IdP user whose email anchors the dev-lead persona, and stand in for the chart's own ClickHouse migration hook. Refs constructorfabric#2243 Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
… by path Both images now `pip install` the seeder, so every runner invokes a program on PATH — `insight-seed <step>` — instead of a module resolved out of a working directory. The Job's command loses its shell wrapper and its `workingDir`, and the toolbox stops listing the database drivers by hand: they come from the package's own metadata, which cannot drift from it. That removes the last two places reaching into the source tree by path: * The Keycloak realm generator moves into the package as `insight_seed.keycloak_realm`. It was building the realm from the seeder's roster through a `sys.path.insert`, which is what a shared roster looks like when the two halves live apart — a realm and a person table that disagree produce a login that authenticates and then resolves to nobody. It ships as a second entry point, `insight-seed-realm`, and `dev-compose.sh` runs it through `uv run --project` rather than reaching for the directory. * The test bootstrap is gone. `tests/` imports `insight_seed` like anything else does, with no `sys.path` surgery and no stubbed driver modules, because the package is installed in the environment the tests run in. dbt moves from a hard dependency to a `silver` extra. No module here imports it: the silver step shells out to the ingestion tree's scripts, and dbt belongs to THAT environment — the toolbox image installs it directly, and the extra covers a host run. As a hard dependency it made every install of this package, including the one a realm generation needs, resolve dbt-core. Installing the package also broke an assumption the generated artifacts were resting on: `manifest.json` and `PROFILE.md` were anchored to the package's own directory, which is wherever pip put it — for the cluster Job, site-packages, where the write failed with EACCES after the seed had already run. Both now resolve against the working directory, with `SEED_MANIFEST_PATH` naming the manifest explicitly where that is not writable (the Job points it at /tmp, and its log carries the document anyway). Compose is unaffected: its working directory is the bind-mounted seeder directory the stand suite reads. Two more, from running it: * `seed-stand.sh` takes `--context` and prints the resolved context in its banner. It inherited the ambient kube context, and an ambient context can change between two runs of a script that applies Jobs to clusters. * The identity existence check no longer formats a column name into its SQL. Two complete statements selected by key: every statement the module executes is now a constant, which is the only form a reader — or a scanner — can confirm at a glance. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Bringing the stand up now generates the Keycloak realm through the seed package's own entry point, which dev-compose.sh runs with uv. The API job already installs it for the suite; this job needs it for the bring-up itself, and would otherwise fail on a missing tool rather than on anything it tests. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Two that mattered, both introduced by this branch: * The realm generator kept a tenant fallback while the seeder started requiring one. Generate a realm with the variable unset and the users carry a tenant claim the seed never writes rows for — they authenticate and then resolve to nobody, the exact failure a shared roster exists to prevent. It now reads the tenant through the same parser the seed does, so the two cannot disagree. * Preflight's foreign-silver scan caught every exception and read the result as a clean stand, which would then TRUNCATE the reset targets unexamined. A fresh stand needs no such tolerance — `system.columns` answers empty for databases that do not exist — so anything raised there is a scan that failed, and it is now a refusal. Then the small ones: `--deadline` is validated before it becomes both the pod ceiling and this script's polling budget; the tests set `IDP_SOURCE_TYPE` rather than defaulting it, so a value in the developer's shell cannot decide what they assert; and four stale module references and a code span left over from the package move. Not taken: an advisory lock around the identity step. Two concurrent seed runs against one tenant can both pass the existence check and append duplicate observations — true, but the read path resolves by newest observation, the duplicates are inert, and concurrent runs of a one-shot demo-data tool are not a case worth serialising a whole step for. Reconsider it if the seeder ever runs unattended. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
A local Studio version bump and a punctuation change in its config README were swept into the previous commit by the hook's stash/restore. Neither has anything to do with the seeder; restore them to main so the branch carries only its own subject. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The gitops `keycloak-realm` target still invoked the generator by its old path, so `make deploy ENV=local` would have failed at realm generation. It now runs the installed console script through `uv run --project`, requires `uv` explicitly, and reads `global.tenantDefaultId` out of the env's values — a missing tenant is an error naming the file, not a realm whose users authenticate and resolve to nobody. Two more holes in the refusals: - The silver step consults the persons signal too. Scanning only for CROSS-tenant silver rows can never fire on a single-tenant stand, which is the stand most likely to be pointed at by mistake, and `--step silver` skipped the persons check entirely while truncating 21 relations. - The anchor date is resolved from the clock once per process. Two callers each reading `now()` disagree across a UTC midnight, and the manifest would then describe a window the rows do not sit in. Tests cover both, plus the reset surface the operator is shown before a truncating step. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The seeder was going to ride along in the toolbox. That image runs migrations against real stands, and a demo-data generator has no business being installed there — one `docker run` away from writing a fake organisation into a stand that holds a real one. So it gets an image of its own. Same build context (`src/ingestion`, so the DDL and gold-build scripts the silver step shells out to travel with it, at one version), a narrower COPY set, and `insight-seed` as the entry point. The toolbox stops carrying it: `tools/seed/` is excluded there through a per-Dockerfile `Dockerfile.dockerignore`, which applies to that image only, so both can build from the same context and get different content. Published beside the toolbox — per-arch matrix, digest merge, provenance attestation — and pinned into the chart as `ingestion.seedImage`, which is what `seed-stand.sh` now discovers. The chart never renders it; nothing in a deployed release runs the seeder. It is published state, so an operator can ask a stand which seeder matches it. The amd64 leg validates before pushing: the console script resolves, every step module imports (the whole driver set, which is exactly what was missing when the seeder could not run on a stand at all), the package's tests pass, and dbt is present for the silver step's subprocess. The tests run inside the shipped image rather than in ci.yml's Python matrix — they are stdlib `unittest` and touch no database, so what gets tested is what gets published. Compose builds the same Dockerfile locally, and the nightly Trivy matrix scans the published image. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The rig landed on main while this branch was open and calls the generator by its old path — `python3 deploy/compose/keycloak/gen-realm.py`, a file this branch deletes. It runs through `uv run --project` now, the same way dev-compose.sh and the gitops `keycloak-realm` target invoke it, and the job installs uv for it. The generator also requires `TENANT_DEFAULT_ID` rather than defaulting to a stand's, so the rig names one. Which tenant is immaterial here — person resolution is keyed by email — but every realm user carries the claim and a realm that will not build is the cheaper failure. The workflow's paths filter followed the roster to its new home, so a change to the people the realm is built from still re-runs the suite. Verified: the rig's exact invocation produces the realm it expects — 27 users, both clients, the three rig redirect URIs replacing the compose defaults — and `kc-realm-overlay.py` consumes it unchanged, emitting both realms with the rig's 9 test users layered on. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
A generator that truncates a relation the registry does not list is a runtime failure: `truncate` refuses an unregistered target, so the silver step would have died partway through, and preflight would have scanned one relation short of what the step actually clears. `class_task_issuetypes` arrived on main while this branch was open. The registry test is what caught it — which is the reason it enumerates the generators' call sites rather than trusting a name pattern. PROFILE.md regenerated: `seed_revision` is a content hash, so it moves with any change to what the seed writes. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The seed and merge-seed jobs were written against the pins that were current when this branch opened. The actions group has moved since, so replaying the branch would have reintroduced eight superseded versions in the two jobs nobody else touches — checkout, buildx, login, build-push, upload/download artifact, metadata and attest — plus the realm generator's uv setup in the authenticator lane. Every pin this branch adds now matches what the rest of the file uses. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/ingestion/tools/seed/tests/test_preflight.py (1)
238-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the default value, not object identity.
Use
assertEqual(base.DEFAULT_SEED_DAYS, config.DEFAULT_SEED_DAYS)here. Object identity covers the current assignment, but the contract is the shared default value after refactoring.🤖 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/tools/seed/tests/test_preflight.py` around lines 238 - 246, Update test_the_generators_and_the_manifest_read_the_same_window to compare base.DEFAULT_SEED_DAYS and config.DEFAULT_SEED_DAYS by value with assertEqual instead of object identity with assertIs; keep the existing parsing and function identity assertions unchanged.
🤖 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 `@src/ingestion/tools/seed/insight_seed/config.py`:
- Around line 129-145: Update parse_mariadb and parse_clickhouse to parse
MARIADB_PORT and CLICKHOUSE_HTTP_PORT inside validation handling, reject
non-numeric and out-of-range values outside 1–65535, and raise EnvContractError
mentioning the relevant environment variable instead of propagating ValueError.
- Around line 81-83: Update the ClickHouse configuration and client setup to
support TLS: make the ClickHouse URL scheme and TLS settings configurable, pass
the corresponding secure connection options (including the password) to
clickhouse_connect.get_client, and preserve the configured endpoint/port. In
src/ingestion/tools/seed/insight_seed/config.py lines 81-83, update
ClickHouse.url; in src/ingestion/tools/seed/seed-stand.sh lines 224-233, forward
the stand endpoint to the Job without rejecting https:// URLs.
In `@src/ingestion/tools/seed/README.md`:
- Around line 58-60: Update the credential-handling statement in the seed
documentation to clarify that seed-stand.sh temporarily decodes database_url to
derive IDENTITY_DB, so credentials pass through the shell process but are
neither printed nor embedded in the Job manifest. Preserve the existing
explanation that the Job runs as the application MariaDB user rather than root.
---
Nitpick comments:
In `@src/ingestion/tools/seed/tests/test_preflight.py`:
- Around line 238-246: Update
test_the_generators_and_the_manifest_read_the_same_window to compare
base.DEFAULT_SEED_DAYS and config.DEFAULT_SEED_DAYS by value with assertEqual
instead of object identity with assertIs; keep the existing parsing and function
identity assertions unchanged.
🪄 Autofix
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: 807474da-da7f-4990-802a-062a617cad0d
📒 Files selected for processing (71)
.env.compose.example.github/dependabot.yml.github/workflows/authenticator.yml.github/workflows/build-images.yml.github/workflows/e2e-stand.yml.github/workflows/trivy-images.yml.gitignoreCONTRIBUTING.mdREADME.mdcharts/insight/values.yamldeploy/HELM_DEPLOY.mddeploy/compose/insight-init.shdeploy/compose/keycloak/README.mddeploy/compose/ui-tests.Dockerfiledeploy/gitops/Makefiledeploy/gitops/environments/local/values.yaml.templatedeploy/seed/Dockerfiledeploy/seed/README.mddeploy/seed/pyproject.tomldeploy/seed/seed.pydeploy/seed/test_identity.pydev-compose.shdocker-compose.ymldocs/TESTING.mdsrc/backend/services/authenticator/tests/kc-realm-overlay.pysrc/backend/services/authenticator/tests/run-e2e.shsrc/ingestion/.dockerignoresrc/ingestion/README.mdsrc/ingestion/tools/seed/Dockerfilesrc/ingestion/tools/seed/PROFILE.mdsrc/ingestion/tools/seed/README.mdsrc/ingestion/tools/seed/insight_seed/__init__.pysrc/ingestion/tools/seed/insight_seed/__main__.pysrc/ingestion/tools/seed/insight_seed/analytics.pysrc/ingestion/tools/seed/insight_seed/config.pysrc/ingestion/tools/seed/insight_seed/generators/__init__.pysrc/ingestion/tools/seed/insight_seed/generators/ai.pysrc/ingestion/tools/seed/insight_seed/generators/base.pysrc/ingestion/tools/seed/insight_seed/generators/collab.pysrc/ingestion/tools/seed/insight_seed/generators/crm.pysrc/ingestion/tools/seed/insight_seed/generators/git.pysrc/ingestion/tools/seed/insight_seed/generators/hr.pysrc/ingestion/tools/seed/insight_seed/generators/people.pysrc/ingestion/tools/seed/insight_seed/generators/support.pysrc/ingestion/tools/seed/insight_seed/generators/task.pysrc/ingestion/tools/seed/insight_seed/golden_metrics.pysrc/ingestion/tools/seed/insight_seed/identity.pysrc/ingestion/tools/seed/insight_seed/keycloak_realm.pysrc/ingestion/tools/seed/insight_seed/manifest.pysrc/ingestion/tools/seed/insight_seed/preflight.pysrc/ingestion/tools/seed/insight_seed/profile_md.pysrc/ingestion/tools/seed/insight_seed/profiles.pysrc/ingestion/tools/seed/insight_seed/render_profile.pysrc/ingestion/tools/seed/insight_seed/silver.pysrc/ingestion/tools/seed/pyproject.tomlsrc/ingestion/tools/seed/seed-job.yaml.tplsrc/ingestion/tools/seed/seed-stand.shsrc/ingestion/tools/seed/tests/__init__.pysrc/ingestion/tools/seed/tests/test_identity.pysrc/ingestion/tools/seed/tests/test_preflight.pysrc/ingestion/tools/toolbox/Dockerfilesrc/ingestion/tools/toolbox/Dockerfile.dockerignoretests/generate_schemas.pytests/lib/insight_stand/__init__.pytests/lib/insight_stand/manifest.pytests/lib/insight_stand/personas.pytests/pyproject.tomltests/stand/README.mdtests/stand/api/identity/test_internal.pytests/stand/conftest.pytests/versions.yaml
💤 Files with no reviewable changes (5)
- deploy/seed/README.md
- deploy/seed/test_identity.py
- deploy/seed/pyproject.toml
- deploy/seed/Dockerfile
- deploy/seed/seed.py
🚧 Files skipped from review as they are similar to previous changes (56)
- deploy/gitops/environments/local/values.yaml.template
- src/ingestion/README.md
- tests/lib/insight_stand/init.py
- src/ingestion/tools/seed/insight_seed/init.py
- src/ingestion/tools/seed/insight_seed/generators/people.py
- tests/versions.yaml
- .github/dependabot.yml
- charts/insight/values.yaml
- src/ingestion/tools/seed/insight_seed/profiles.py
- src/ingestion/tools/seed/PROFILE.md
- src/ingestion/tools/seed/insight_seed/analytics.py
- src/ingestion/tools/toolbox/Dockerfile
- tests/stand/api/identity/test_internal.py
- src/ingestion/tools/seed/insight_seed/generators/git.py
- docs/TESTING.md
- deploy/compose/insight-init.sh
- .github/workflows/trivy-images.yml
- tests/stand/conftest.py
- .github/workflows/e2e-stand.yml
- src/ingestion/tools/seed/insight_seed/generators/init.py
- src/ingestion/tools/seed/insight_seed/generators/crm.py
- .env.compose.example
- src/ingestion/tools/seed/pyproject.toml
- tests/lib/insight_stand/personas.py
- deploy/compose/ui-tests.Dockerfile
- .github/workflows/authenticator.yml
- src/ingestion/tools/seed/tests/test_identity.py
- src/ingestion/.dockerignore
- src/backend/services/authenticator/tests/run-e2e.sh
- tests/pyproject.toml
- src/ingestion/tools/seed/insight_seed/generators/collab.py
- .gitignore
- src/ingestion/tools/seed/insight_seed/golden_metrics.py
- src/backend/services/authenticator/tests/kc-realm-overlay.py
- tests/stand/README.md
- src/ingestion/tools/seed/insight_seed/generators/base.py
- src/ingestion/tools/seed/insight_seed/generators/hr.py
- src/ingestion/tools/seed/insight_seed/keycloak_realm.py
- docker-compose.yml
- tests/generate_schemas.py
- deploy/HELM_DEPLOY.md
- src/ingestion/tools/seed/insight_seed/generators/ai.py
- deploy/compose/keycloak/README.md
- src/ingestion/tools/seed/insight_seed/manifest.py
- src/ingestion/tools/seed/insight_seed/render_profile.py
- src/ingestion/tools/seed/insight_seed/preflight.py
- README.md
- dev-compose.sh
- src/ingestion/tools/seed/insight_seed/generators/support.py
- src/ingestion/tools/seed/insight_seed/silver.py
- src/ingestion/tools/seed/insight_seed/profile_md.py
- src/ingestion/tools/seed/insight_seed/main.py
- tests/lib/insight_stand/manifest.py
- src/ingestion/tools/seed/insight_seed/generators/task.py
- src/ingestion/tools/seed/Dockerfile
- src/ingestion/tools/seed/insight_seed/identity.py
Comment on lines
+81
to
+83
| @property | ||
| def url(self) -> str: | ||
| return f"http://{self.host}:{self.http_port}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/ingestion/tools/seed/insight_seed/config.py --items all
rg -n -C 4 --type py 'ClickHouse|\.url\b|clickhouse_connect|get_client' \
src/ingestion/tools/seed
rg -n -C 4 'CLICKHOUSE_URL|https://|CLICKHOUSE_HTTP_PORT' \
src/ingestion/tools/seed
sed -n '1,220p' src/ingestion/tools/seed/pyproject.tomlRepository: constructorfabric/insight
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- config.py ---\n'
sed -n '60,146p' src/ingestion/tools/seed/insight_seed/config.py
printf '\n--- preflight.py ClickHouse branch ---\n'
sed -n '261,286p' src/ingestion/tools/seed/insight_seed/preflight.py
printf '\n--- silver.py ClickHouse client/env ---\n'
sed -n '68,98p' src/ingestion/tools/seed/insight_seed/silver.py
printf '\n--- seed-stand.sh ClickHouse URL handling ---\n'
sed -n '212,240p' src/ingestion/tools/seed/seed-stand.sh
printf '\n--- shell URL matches ---\n'
rg -n -C 2 'CLICKHOUSE_URL|https://|CM_CLICKHOUSE|PLATFORM.*CLICKHOUSE|CLICKHOUSE_HTTP_PORT' src/ingestion/tools/seed/seed-stand.sh
printf '\n--- clickhouse connect kwargs in source ---\n'
python3 - <<'PY'
from pathlib import Path
for path in Path("src/ingestion/tools/seed").rglob("*.py"):
text = path.read_text()
if "clickhouse_connect" in text:
for i,line in enumerate(text.splitlines(),1):
if "get_client" in line:
start=max(1,i-8); end=min(len(text.splitlines()),i+18)
ls=text.splitlines(); print(f"\nFILE {path}:{start}-{end}")
for n in range(start,end+1): print(f"{n}: {ls[n-1]}")
PYRepository: constructorfabric/insight
Length of output: 254
🌐 Web query:
clickhouse_connect get_client secure protocol https parameters
💡 Result:
To establish a secure HTTPS connection using the clickhouse-connect Python driver, you can use the get_client method with specific parameters to enable and configure TLS [1][2]. Core Connection Parameters To enable HTTPS, you can use the secure parameter or specify the interface [1][2]. - secure: Set to True to enable HTTPS/TLS [1][2]. - interface: Setting this to "https" also enables HTTPS [1]. - port: When using HTTPS, the port defaults to 8443 if not specified [2][3]. If using port 443 or 8443, the driver may automatically infer the use of HTTPS [1]. Common HTTPS/TLS Configuration Parameters The following parameters are frequently used to manage secure connections [1][2]: - verify: A boolean or string. Defaults to True, which validates the server's TLS certificate [1][2]. If set to "proxy", it enables proxy TLS mode [1]. - ca_cert: A string providing the file path to a custom CA bundle [1]. You can set this to "certifi" to use the CA bundle provided by the certifi package [1]. - client_cert: A string path to a PEM-formatted client certificate (used for mutual TLS) [1][2]. - client_cert_key: A string path to the private key for the client certificate if it is not included in the client_cert file [1][2]. - server_host_name: Used when the hostname in the TLS certificate (CN/SNI) differs from the actual host, such as when connecting through a tunnel or proxy [1][2][4]. - tls_mode: Controls advanced TLS behavior (e.g., "mutual", "proxy", or "strict") [1][2]. The default None behaves as "mutual" if a client certificate is provided [1]. Example Usage To connect to a secure server, such as ClickHouse Cloud: import clickhouse_connect client = clickhouse_connect.get_client( host="HOSTNAME.clickhouse.cloud", port=8443, secure=True, username="default", password="your_password") For advanced mutual TLS or custom certificate scenarios: client = clickhouse_connect.get_client( host="your.secure.host", port=8443, secure=True, verify=True, ca_cert="/path/to/ca.crt", client_cert="/path/to/client.crt", client_cert_key="/path/to/client.key") For more detailed information, consult the official ClickHouse Connect driver API documentation [1].
Citations:
- 1: https://clickhouse.com/docs/integrations/language-clients/python/driver-api
- 2: https://github.com/ClickHouse/clickhouse-docs/blob/main/docs/integrations/language-clients/python/driver-api.md
- 3: https://clickhouse.com/docs/integrations/language-clients/python
- 4: https://github.com/ClickHouse/clickhouse-connect/blob/main/examples/ssh_tunnels.py
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- config.py ---'
sed -n '60,146p' src/ingestion/tools/seed/insight_seed/config.py
echo
echo '--- preflight.py ClickHouse branch ---'
sed -n '261,286p' src/ingestion/tools/seed/insight_seed/preflight.py
echo
echo '--- silver.py ClickHouse client/env ---'
sed -n '68,98p' src/ingestion/tools/seed/insight_seed/silver.py
echo
echo '--- seed-stand.sh ClickHouse URL handling ---'
sed -n '212,240p' src/ingestion/tools/seed/seed-stand.sh
echo
echo '--- shell URL matches ---'
rg -n -C 2 'CLICKHOUSE_URL|https://|CM_CLICKHOUSE|PLATFORM.*CLICKHOUSE|CLICKHOUSE_HTTP_PORT' src/ingestion/tools/seed/seed-stand.sh
echo
echo '--- clickhouse connect kwargs in py source ---'
python3 - <<'PY'
from pathlib import Path
for path in Path("src/ingestion/tools/seed").rglob("*.py"):
text = path.read_text()
if "clickhouse_connect" in text:
lines = text.splitlines()
for i,line in enumerate(lines,1):
if "get_client" in line:
start=max(1,i-8); end=min(len(lines),i+18)
print(f"\nFILE {path}:{start}-{end}")
for n in range(start,end+1): print(f"{n}: {lines[n-1]}")
PYRepository: constructorfabric/insight
Length of output: 11115
Support TLS for ClickHouse connections.
The seeder builds ClickHouse.url as http:// and passes CLICKHOUSE_PASSWORD to clients and script env without TLS settings. get_client default-connects on HTTP/8123, and seed-stand.sh rejects https:// ClickHouse URLs before the Job starts. Make scheme/TLS settings part of ClickHouse and clickhouse_connect.get_client(...), then forward the stand endpoint instead of forbidding HTTPS.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 82-82: Do not make http calls without encryption
Context: f"http://{self.host}:{self.http_port}"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
📍 Affects 2 files
src/ingestion/tools/seed/insight_seed/config.py#L81-L83(this comment)src/ingestion/tools/seed/seed-stand.sh#L224-L233
🤖 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/tools/seed/insight_seed/config.py` around lines 81 - 83, Update
the ClickHouse configuration and client setup to support TLS: make the
ClickHouse URL scheme and TLS settings configurable, pass the corresponding
secure connection options (including the password) to
clickhouse_connect.get_client, and preserve the configured endpoint/port. In
src/ingestion/tools/seed/insight_seed/config.py lines 81-83, update
ClickHouse.url; in src/ingestion/tools/seed/seed-stand.sh lines 224-233, forward
the stand endpoint to the Job without rejecting https:// URLs.
Source: Linters/SAST tools
Comment on lines
+129
to
+145
| def parse_mariadb(env: Mapping[str, str], *, database: str) -> MariaDb: | ||
| return MariaDb( | ||
| host=env.get("MARIADB_HOST", "mariadb"), | ||
| port=int(env.get("MARIADB_PORT", "3306")), | ||
| user=env.get("MARIADB_USER", "insight"), | ||
| password=env.get("MARIADB_PASSWORD", "insight-local"), | ||
| database=database, | ||
| ) | ||
|
|
||
|
|
||
| def parse_clickhouse(env: Mapping[str, str]) -> ClickHouse: | ||
| return ClickHouse( | ||
| host=env.get("CLICKHOUSE_HOST", "clickhouse"), | ||
| http_port=int(env.get("CLICKHOUSE_HTTP_PORT", "8123")), | ||
| user=env.get("CLICKHOUSE_USER", "insight"), | ||
| password=env.get("CLICKHOUSE_PASSWORD", "insight-local"), | ||
| database=env.get("CLICKHOUSE_DATABASE", "insight"), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/ingestion/tools/seed/insight_seed/config.py --items all
rg -n -C 3 'def parse_(mariadb|clickhouse)|MARIADB_PORT|CLICKHOUSE_HTTP_PORT|int\(' \
src/ingestion/tools/seed/insight_seed/config.py
fd -a '^pyproject\.toml$' src/ingestion | xargs -r -n1 sh -c '
echo "== $0"
rg -n -C 2 "requires-python|target-version" "$0" || true
'Repository: constructorfabric/insight
Length of output: 6467
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l src/ingestion/tools/seed/insight_seed/config.py
sed -n '1,90p' src/ingestion/tools/seed/insight_seed/config.py
sed -n '125,210p' src/ingestion/tools/seed/insight_seed/config.py
rg -n "EnvContractError|MARIADB_PORT|CLICKHOUSE_HTTP_PORT|MARIADB_ANALYTICS_DB|MARIADB_DB|clickhouse|mariadb" src/ingestion/tools/seed -g '*.py'Repository: constructorfabric/insight
Length of output: 17943
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l src/ingestion/tools/seed/insight_seed/preflight.py
sed -n '330,360p' src/ingestion/tools/seed/insight_seed/preflight.py
sed -n '360,415p' src/ingestion/tools/seed/insight_seed/preflight.py
sed -n '380,415p' src/ingestion/tools/seed/insight_seed/identity.py
python3 - <<'PY'
from pathlib import Path
import ast
mod = ast.parse(Path("src/ingestion/tools/seed/insight_seed/config.py").read_text())
for node in ast.walk(mod):
if isinstance(node, ast.expr) and isinstance(node.value, ast.Call):
func = node.value.func
name = getattr(ast.unparse(func), "_ast_type", None) or getattr(func, "attr", None) or getattr(func, "id", None)
if name == "int" or (hasattr(func, "attr") and func.attr == "get"):
if any("PORT" in getattr(arg, "s", "") or getattr(arg, "value", "") in ("CLICKHOUSE_HTTP_PORT", "MARIADB_PORT") for arg in node.value.args if isinstance(arg, ast.Constant)):
print(ast.unparse(node), "line", node.lineno)
PYRepository: constructorfabric/insight
Length of output: 4950
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '300,360p' src/ingestion/tools/seed/insight_seed/preflight.py
sed -n '355,410p' src/ingestion/tools/seed/insight_seed/preflight.py
# Deterministic semantics probe for invalid and out-of-range port strings without importing the module.
python3 - <<'PY'
cases = [
("MARIADB_PORT", "not-a-number", "mariadb"),
("CLICKHOUSE_HTTP_PORT", "-1", "clickhouse"),
("CLICKHOUSE_HTTP_PORT", "99999", "clickhouse"),
("MARIADB_PORT", "0", "mariadb"),
("CLICKHOUSE_HTTP_PORT", "0", "clickhouse"),
]
for var, value, default in cases:
try:
int(value)
problem = None
except Exception as exc:
problem = type(exc).__name__
print({
"input": value,
"variable": var,
"exception_on_int": problem,
"valid_range": isinstance(value, str) and value.lstrip("-").isdigit() and 0 <= int(value) <= 65535,
"default_used": value is None,
})
PYRepository: constructorfabric/insight
Length of output: 5892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '80,110p' src/ingestion/tools/seed/insight_seed/identity.py
python3 - <<'PY'
import ast
from pathlib import Path
src = Path("src/ingestion/tools/seed/insight_seed/config.py").read_text()
tree = ast.parse(src)
for node in tree.body:
if isinstance(node, ast.ClassDef) and node.name in {"MariaDb", "ClickHouse"}:
print(f"--- {node.name} line {node.lineno} ---")
for item in node.body:
if isinstance(item, ast.FunctionDef):
print(f"{item.name} line {item.lineno}: {ast.get_source_segment(src, item)}")
for item in node.body:
if isinstance(item, ast.AnnAssign) and isinstance(item.target, ast.Name):
seg = ast.get_source_segment(src, item)
print(f"field {node.name}.{item.target.id}: {seg}")
PYRepository: constructorfabric/insight
Length of output: 1665
Normalize port parsing failures to EnvContractError.
MARIADB_PORT and CLICKHOUSE_HTTP_PORT currently raise a raw ValueError for non-numeric values. Parse them in a try/except, reject out-of-range ports like -1 or >65535, and raise EnvContractError with the variable name so operator-facing validation stays typed.
🤖 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/tools/seed/insight_seed/config.py` around lines 129 - 145,
Update parse_mariadb and parse_clickhouse to parse MARIADB_PORT and
CLICKHOUSE_HTTP_PORT inside validation handling, reject non-numeric and
out-of-range values outside 1–65535, and raise EnvContractError mentioning the
relevant environment variable instead of propagating ValueError.
Source: Coding guidelines
Comment on lines
+58
to
+60
| Credentials never pass through the script, and the Job runs as the application | ||
| MariaDB user rather than root: the umbrella already grants that user everything | ||
| the seed writes. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Correct the credential-handling statement.
seed-stand.sh temporarily decodes the identity service database_url to derive IDENTITY_DB. That URL can contain database credentials. The script does not print it or place it in the Job manifest, but credentials do pass through the shell process.
Proposed fix
-Credentials never pass through the script, and the Job runs as the application
-MariaDB user rather than root: the umbrella already grants that user everything
-the seed writes.
+Database passwords do not enter the Job manifest or shell output. The script
+temporarily decodes the identity service database URL only to derive its database
+name. The Job runs as the application MariaDB user rather than root: the umbrella
+already grants that user everything the seed writes.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Credentials never pass through the script, and the Job runs as the application | |
| MariaDB user rather than root: the umbrella already grants that user everything | |
| the seed writes. | |
| Database passwords do not enter the Job manifest or shell output. The script | |
| temporarily decodes the identity service database URL only to derive its database | |
| name. The Job runs as the application MariaDB user rather than root: the umbrella | |
| already grants that user everything the seed writes. |
🤖 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/tools/seed/README.md` around lines 58 - 60, Update the
credential-handling statement in the seed documentation to clarify that
seed-stand.sh temporarily decodes database_url to derive IDENTITY_DB, so
credentials pass through the shell process but are neither printed nor embedded
in the Job manifest. Preserve the existing explanation that the Job runs as the
application MariaDB user rather than root.
ktursunov
disabled auto-merge
August 7, 2026 02:10
ktursunov
enabled auto-merge
August 7, 2026 02:10
hello1101n
approved these changes
Aug 7, 2026
This was referenced Aug 10, 2026
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
3 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.
Closes #2243.
deploy/seedwas compose-only, so a chart-deployed stand starts empty: every login is refused and every dashboard reads "No data". This makes populating one a single command.What changed
The seeder moved into the ingestion tree —
src/ingestion/tools/seed. The silver step shells out to the ingestion tree's own DDL and gold-build scripts, so living in that tree is what lets one image carry both at one version, with no chance of the seeder and the migration SQL drifting apart.It is published as its own image,
insight-seed— deliberately not the toolbox. The toolbox runs migrations against real stands, and a demo-data generator has no business being installed there. Same build context, a narrower COPY set,insight-seedas the entry point; the toolbox excludestools/seed/through a per-DockerfileDockerfile.dockerignore, so both build from the same context and get different content. CI publishes it beside the toolbox (per-arch matrix, digest merge, provenance attestation), the nightly Trivy matrix scans it, and compose builds the same Dockerfile locally. The amd64 leg validates before pushing: the console script resolves, every step module imports (the whole driver set — exactly what was missing when the seeder could not run on a stand at all), the package's tests pass, and dbt is present for the silver step's subprocess. No chart template — nothing in a deployed release runs the seeder.The tree is a package plus its tests —
insight_seed/andtests/. Every caller installs it and invokes a program rather than a module in a directory:insight-seed <step>seeds,insight-seed-realmgenerates the compose Keycloak realm from the same roster.manifest.jsonandPROFILE.mdstay at the tool root, where their readers already name them.The environment contract fails loudly.
TENANT_DEFAULT_IDandMARIADB_ANALYTICS_DBlose their defaults — rows under the wrong tenant are invisible to every login while the run still reports success, and which database holds the catalogue tables is a per-stand fact (compose uses a database of its own; a chart stand keeps them inmariadb.database). A newpreflightmodule answers every answerable question before anything is written and reports the whole list at once.It refuses rather than damages. The identity step is additive and marks its rows, so a tenant already holding other rows is refused. The silver step is not additive — it
TRUNCATEs every table it writes, across all tenants — so a stand holding another tenant's rows anywhere in that reset surface is refused too. The reset surface is one registry thattruncateitself enforces, kept in step with the generator call sites by a test.SEED_FORCE=1overrides either, deliberately.seed-stand.shdiscovers, renders, applies, follows. Coordinates come from the stand's own<release>-platformConfigMap and its Secrets; the image fromingestion.seedImage, which CI pins on publish. It is published state, not deployed state — the chart never renders it; it is there so an operator can ask a stand which seeder matches it. Credentials never pass through the shell — the Job references the release's Secret by key and runs as the application MariaDB user, which the umbrella already grants everything the seed writes. It is a plain Job, not a chart hook, so a failed seed never gates or rolls back a release.--dry-runprintsseed-job.yaml.tplrendered — the reference manifest and the one applied are the same file.Along the way: the second-tenant fixture is gated (it aborts identity-resolution's scheduled projection on a cluster and is only wanted on compose, where the stand suite asserts against it); the activity window is read in one place so the manifest cannot report a window the rows do not sit in; identity re-runs are idempotent again (
seed_persons/seed_person_namesrelied on anINSERT IGNOREcollision that migration 004 removed, so each run appended duplicate email and name observations); and host build artefacts are kept out of both images.Acceptance criteria
pip.--dry-rungives the manifest; the script applies it.metric_definitionsrather than assumed, and the database it looked in named in the error.--emailmust already exist in the realm. Documented as a prerequisite in the seeder README,HELM_DEPLOY.md(new Step 7) andCONTRIBUTING.md. Generating the realm from the seeder's own roster is follow-up work, not part of this change.Verification
Run against a local single-node cluster with the umbrella chart deployed and external MariaDB/ClickHouse, using an image built from this branch:
kubectl apply --dry-run=client;--step identitycompletes in seconds (26 people, their login rows, names, org-chart edges and account maps); re-running it inserts nothing;--step analyticswrites the tenant-scoped catalogue row — which is what proves the discovered analytics database is the right one;--step silverend to end against a throwaway ClickHouse of the pinned version: placeholders → generators → migrations →dbt run --select tag:gold, 29,306 rows across 19 silver tables,PASS=18 WARN=0 ERROR=0; seeding twice in a row works.Locally:
ruff check/ruff format --checkclean,mypyclean, 39 unit tests (which CI now runs inside the published image),PROFILE.mdregenerated and its staleness check green,docker compose --profile seed configand theseed-sampleimage build, and the realm generator still builds its 27 users from the shared roster.The compose path is unchanged in behaviour:
docker-compose.ymlnow passesMARIADB_ANALYTICS_DBandSEED_CROSS_TENANT_FIXTUREexplicitly, which it previously inherited from the code defaults this PR removes.Note for reviewers
The diff is large because a relocation cannot be split from the rewiring that follows it — every caller has to move in the same commit or the tree is broken in between. The substance is in
src/ingestion/tools/seed/:config.py,preflight.py,seed-stand.shandseed-job.yaml.tplare new; everything else under that path is the moved package.One gap left open on purpose: the package is not registered in
scripts/ci/components.py, so it is outside the per-component coverage gate. Its suite covers the pure half — the environment contract, the SQL each guard issues, the messages a refusal carries — and the I/O half it deliberately does not touch would put the component well under the 80% line. The tests still run on every ingestion change, in the image build. Raising them to the gate's bar is worth doing separately.Summary by CodeRabbit
New Features
Bug Fixes
Documentation