fix(RHCLOUD-47878): transform widget item IDs during data migration and adding data integrity check#143
Open
tahmidefaz wants to merge 5 commits into
Open
Conversation
The migration script was copying JSONB columns (sm/md/lg/xl) as-is,
preserving chrome-service's "shortKey#shortKey" format (e.g.,
"rhel#rhel"). The widget-layout-backend /widget-mapping endpoint
returns keys in "{scope}-{module}" format (e.g.,
"landing-./RhelWidget"), causing a key mismatch that prevents the
frontend from rendering widgets.
Add WIDGET_ID_MAP (16 entries from docs/WIDGET_MIGRATION.md) and
transform_widget_ids() to rewrite "i" fields during export. Update
both migration docs with schema translation notes, expected output,
and post-import verification queries.
Tested end-to-end locally with seeded chrome-service data covering
all 16 widget types.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughAdds WIDGET_ID_MAP and transform_widget_ids(items); integrates JSONB "i" rewriting into do_export (abort on unmapped IDs); writes/validates an INSERT-count META_FILE; adds import ChangesWidget ID Transformation for Migration
sequenceDiagram
participant Operator
participant ExportScript as widget-migration-script.py
participant SourceDB as Postgres(source)
participant Local as DebugHost
participant TargetPod as debug-container
Operator->>ExportScript: run export
ExportScript->>SourceDB: query dashboard_templates rows
ExportScript->>ExportScript: transform_widget_ids(sm/md/lg/xl)
ExportScript-->>Operator: "Transformed widget IDs in JSONB columns"
ExportScript->>Local: write widget_migration.sql and widget_migration.meta
Operator->>Local: base64-encode and stream to TargetPod
Local->>TargetPod: base64 decode -> /tmp/widget_migration.sql
Operator->>TargetPod: verify INSERT count vs /tmp/widget_migration.meta
Operator->>TargetPod: run import (--yes to skip prompt)
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs/chrome-data-migration/MIGRATION_PROD.md`:
- Around line 549-554: The verification SQL only inspects the sm column but the
migration updates sm, md, lg, and xl; update the psql checks to inspect all four
layout columns in dashboard_templates (sm, md, lg, xl): for the sample row
query, retrieve the first widget id from each column (e.g., sm->0->>'i',
md->0->>'i', lg->0->>'i', xl->0->>'i') so you can visualize FEO-format IDs
across layouts, and for the legacy-id count, change the WHERE clause to check
any of sm::text, md::text, lg::text, or xl::text for '%#%' so the query returns
0 only when none of the layout columns contain old-format IDs.
In `@docs/chrome-data-migration/MIGRATION_STAGING.md`:
- Around line 311-316: The fenced code block under the "Expected output" section
that currently starts with ``` and contains the lines "Found N active
dashboard_templates rows", "Fetched N rows (with user_id resolved)",
"Transformed widget IDs in JSONB columns (sm/md/lg/xl)", and "Generated N INSERT
statements in /tmp/widget_migration.sql" should include a language identifier to
satisfy MD040; change the opening fence from ``` to ```text so the block becomes
a text fenced block.
- Around line 473-478: The verification only checks the sm column but the
migration transforms ids in sm, md, lg, and xl; update the validation to inspect
all layout columns on dashboard_templates by selecting first widget ids from
each of sm, md, lg, xl (e.g., sm->0->>'i', md->0->>'i', lg->0->>'i',
xl->0->>'i') and change the COUNT(*) query to search for old-format IDs in any
of sm, md, lg, xl (e.g., WHERE sm::text LIKE '%#%' OR md::text LIKE '%#%' OR
lg::text LIKE '%#%' OR xl::text LIKE '%#%') so unmigrated IDs in other
breakpoints are detected.
In `@docs/chrome-data-migration/widget-migration-script.py`:
- Line 350: The print uses an unnecessary f-string prefix: change the call
print(f"Transformed widget IDs in JSONB columns (sm/md/lg/xl)") to a normal
string, e.g. print("Transformed widget IDs in JSONB columns (sm/md/lg/xl)"),
removing the leading 'f' so the f-string linter warning (F541) is resolved;
locate the statement matching print(f"Transformed widget IDs in JSONB columns
(sm/md/lg/xl)") in widget-migration-script.py and update it accordingly.
- Around line 112-115: The code assumes item["i"] is a string when computing
raw_id and short_key which causes AttributeError for non-string IDs; update the
handling around raw_id/short_key (the item["i"] read and the subsequent split
into short_key used to check WIDGET_ID_MAP) to first validate type (e.g., if not
isinstance(raw_id, str) treat it as unmapped/skippable or coerce to str safely)
and handle the non-string case by logging or recording it as unmapped and
continuing rather than calling split on it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9d0882a4-b24c-4fe2-8078-e948c609875b
📒 Files selected for processing (3)
docs/chrome-data-migration/MIGRATION_PROD.mddocs/chrome-data-migration/MIGRATION_STAGING.mddocs/chrome-data-migration/widget-migration-script.py
tahmidefaz
changed the title
- Add isinstance check + warning log for non-string widget IDs
- Use split("#", 1) to limit splits
- Verification queries now check sm/md/lg/xl instead of just sm
- Remove unnecessary f-string prefix
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (2)
docs/chrome-data-migration/MIGRATION_STAGING.md (2)
311-316:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language identifier to the fenced “Expected output” block.
This still triggers MD040 and can fail markdown lint checks.
Suggested fix
-``` +```text Found N active dashboard_templates rows Fetched N rows (with user_id resolved) Transformed widget IDs in JSONB columns (sm/md/lg/xl) Generated N INSERT statements in /tmp/widget_migration.sql</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/chrome-data-migration/MIGRATION_STAGING.mdaround lines 311 - 316, The
fenced code block showing the "Expected output" (the block containing "Found N
active dashboard_templates rows" / "Generated N INSERT statements in
/tmp/widget_migration.sql") lacks a language identifier and triggers MD040;
update that triple-backtick fence to include a language tag (e.g., use ```text)
so the block becomes a labeled text code fence and will satisfy markdown lint
checks.</details> --- `470-472`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Expand JSON-population verification beyond `sm`.** The check currently validates only `sm`, but migration correctness depends on `sm/md/lg/xl`. This can miss partially migrated layouts. <details> <summary>Suggested SQL update</summary> ```diff -# Verify JSON columns are populated -psql -c "SELECT COUNT(*) FROM dashboard_templates WHERE sm IS NOT NULL AND sm::text != '[]'" +# Verify all layout JSON columns are populated +psql -c "SELECT COUNT(*) FROM dashboard_templates WHERE sm IS NOT NULL AND sm::text != '[]' AND md IS NOT NULL AND md::text != '[]' AND lg IS NOT NULL AND lg::text != '[]' AND xl IS NOT NULL AND xl::text != '[]'" ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/chrome-data-migration/MIGRATION_STAGING.md` around lines 470 - 472, The current verification only checks the dashboard_templates.sm column; update the validation queries to cover all layout JSON columns (sm, md, lg, xl). Add a query that asserts all four columns are populated/non-empty (e.g., require sm, md, lg, xl NOT NULL and not equal to '[]') and also add a complementary query that selects rows where any of sm/md/lg/xl IS NULL or sm/md/lg/xl::text = '[]' to catch partial migrations; reference the dashboard_templates table and the sm, md, lg, xl columns when making these changes. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Duplicate comments:
In@docs/chrome-data-migration/MIGRATION_STAGING.md:
- Around line 311-316: The fenced code block showing the "Expected output" (the
block containing "Found N active dashboard_templates rows" / "Generated N INSERT
statements in /tmp/widget_migration.sql") lacks a language identifier and
triggers MD040; update that triple-backtick fence to include a language tag
(e.g., use ```text) so the block becomes a labeled text code fence and will
satisfy markdown lint checks.- Around line 470-472: The current verification only checks the
dashboard_templates.sm column; update the validation queries to cover all layout
JSON columns (sm, md, lg, xl). Add a query that asserts all four columns are
populated/non-empty (e.g., require sm, md, lg, xl NOT NULL and not equal to
'[]') and also add a complementary query that selects rows where any of
sm/md/lg/xl IS NULL or sm/md/lg/xl::text = '[]' to catch partial migrations;
reference the dashboard_templates table and the sm, md, lg, xl columns when
making these changes.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Enterprise **Run ID**: `b46cc1d0-5eb3-4f38-ae3f-911abdf661b4` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between caf99206b611a02834364c2c9d902814e96a0e8f and cbac741906d2b1316bcff9ce8d3472dc25965191. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `docs/chrome-data-migration/MIGRATION_PROD.md` * `docs/chrome-data-migration/MIGRATION_STAGING.md` * `docs/chrome-data-migration/widget-migration-script.py` </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * docs/chrome-data-migration/MIGRATION_PROD.md </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * docs/chrome-data-migration/widget-migration-script.py </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
oc cp and pipe-based transfers silently truncate large SQL files (~9.5MB). Export now writes a .meta file with expected INSERT count, and preflight-import cross-checks it to catch truncation. Docs updated to use base64 encode/decode for reliable pod file transfers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
tahmidefaz
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/chrome-data-migration/widget-migration-script.py`:
- Around line 285-295: The current loop reading META_FILE only validates when an
"INSERT_COUNT=" line is found, letting malformed or missing keys silently pass;
update the logic in the block that reads META_FILE (the code using META_FILE,
insert_count, all_ok, and check) to detect and treat missing or malformed
INSERT_COUNT as a failure: attempt to open META_FILE, iterate lines searching
for "INSERT_COUNT=", and if no such line is found or parsing to int fails, call
check (or set all_ok accordingly) to mark "INSERT count matches export" as
failed with a clear message (e.g., "missing or malformed INSERT_COUNT in
metadata"), ensuring break/return behavior remains correct when valid value is
found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 877390fc-4ab4-4293-9e6b-2cd715e21fd5
📒 Files selected for processing (3)
docs/chrome-data-migration/MIGRATION_PROD.mddocs/chrome-data-migration/MIGRATION_STAGING.mddocs/chrome-data-migration/widget-migration-script.py
✅ Files skipped from review due to trivial changes (1)
- docs/chrome-data-migration/MIGRATION_PROD.md
Addresses CodeRabbit review: preflight-import silently skipped integrity check when .meta file was present but malformed or missing the key. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/chrome-data-migration/widget-migration-script.py (1)
113-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNon-string widget IDs bypass the unmapped-ID fail-fast path.
Lines 114-116 only warn and continue. That allows malformed
item["i"]values to pass export even though this migration is supposed to abort on unresolved widget IDs. Treat these as unmapped so export fails deterministically.Suggested fix
raw_id = item["i"] if not isinstance(raw_id, str): - print(f" WARNING: skipping non-string widget ID: {raw_id!r} (type={type(raw_id).__name__})") + unmapped.add(str(raw_id)) continue🤖 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 `@docs/chrome-data-migration/widget-migration-script.py` around lines 113 - 116, The code currently just warns and continues when item["i"] is not a str (raw_id), which lets malformed IDs bypass the unmapped-ID failure; instead, treat non-string raw_id values as unmapped by recording them into the same unresolved/unmapped collection used for widget ID resolution (e.g., unmapped_ids or unmapped_widget_ids) and do not silently skip them—preserve the control flow so the existing export-failure logic will run and abort deterministically; update the block handling raw_id (checking isinstance(raw_id, str)) to add the offending raw_id (or a descriptive placeholder) to the unmapped set and then continue, rather than only printing a warning.
🤖 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 `@docs/chrome-data-migration/widget-migration-script.py`:
- Around line 304-305: The preflight currently calls check("Meta file
({META_FILE})", False, ...) when sql_exists but does not update the cumulative
result, so missing .meta doesn't flip the all_ok flag; modify the branch that
checks sql_exists to assign the return of check into all_ok (e.g., all_ok =
all_ok and check(...)) or otherwise combine it (all_ok &= check(...)),
referencing the sql_exists condition, the META_FILE constant/name, the
check(...) call, and the all_ok variable to ensure the preflight fails when the
meta file is absent.
---
Duplicate comments:
In `@docs/chrome-data-migration/widget-migration-script.py`:
- Around line 113-116: The code currently just warns and continues when
item["i"] is not a str (raw_id), which lets malformed IDs bypass the unmapped-ID
failure; instead, treat non-string raw_id values as unmapped by recording them
into the same unresolved/unmapped collection used for widget ID resolution
(e.g., unmapped_ids or unmapped_widget_ids) and do not silently skip
them—preserve the control flow so the existing export-failure logic will run and
abort deterministically; update the block handling raw_id (checking
isinstance(raw_id, str)) to add the offending raw_id (or a descriptive
placeholder) to the unmapped set and then continue, rather than only printing a
warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5dbbcccb-a11b-4f11-82e1-af6142e79c4d
📒 Files selected for processing (1)
docs/chrome-data-migration/widget-migration-script.py
check() result was not assigned to all_ok, so preflight reported READY even when .meta file was absent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/chrome-data-migration/widget-migration-script.py`:
- Around line 114-116: The code currently prints a warning for non-string raw_id
but doesn't mark the item as unmapped, so add the non-string ID to the unmapped
set inside the same branch: in the if not isinstance(raw_id, str) block, after
the print warning, call unmapped.add(raw_id) (or add an appropriate identifier
used by the unmapped set) and then continue; this ensures the later
unmapped-check/abort logic (around the export abort at the section that checks
the unmapped set) will trigger and stop the export for investigation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9be59f9f-d34c-46a6-9501-6e9b9dd61f51
📒 Files selected for processing (1)
docs/chrome-data-migration/widget-migration-script.py
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WIDGET_ID_MAP(16 entries fromdocs/WIDGET_MIGRATION.md) andtransform_widget_ids()to the migration script to rewrite widget item"i"fields in JSONB columns (sm/md/lg/xl) from chrome-service format ("rhel#rhel") to FEO format ("landing-./RhelWidget")/widget-mappingendpoint, causing no widgets to renderTest plan
preflight-export→export→preflight-import→import), verified all widget IDs transformed correctly and zero old-format IDs remain🤖 Generated with Claude Code