refactor: move grafana into dashboards/ - #481
Conversation
- Move grafana/ -> dashboards/grafana/ - Update CI workflow, sync script, tests, and .gitignore - Fix cross-reference links and docs
caohy1988
left a comment
There was a problem hiding this comment.
P0: 0 / P1: 2 / P2: 0 — REQUEST_CHANGES
Fresh review of 47620b8f05926568ba4f94968a248f6935f3fd79 against 120da786c6f47122ac48ebeef5c8dfa9c7cd95b1, using the requested path-migration severity bar.
-
P1 — Keep the legacy credential ignore rules when adding the new paths.
.gitignore:38-42replaces all three old rules. Existing checkouts retain ignored, untracked files undergrafana/when the tracked files move, so previously protectedgrafana/*.json,grafana/datasource.yaml, andgrafana/.local/provisioning/datasources/bigquery.yamlbecome visible togit add .. Those are documented service-account key/provisioning locations, and the JWT runner writes the private key into the datasource YAML. An isolated fixture with harmless marker files reproduces all three paths changing from ignored at base to untracked at this head; the new locations remain correctly ignored. Retain the old rules alongside the new rules so upgrading does not expose existing credentials. No actual credential leak was found in this diff. -
P1 — Complete the documentation and help-path migration.
dashboards/grafana/README.md:5retains../dashboard_v2, which now resolves to nonexistentdashboards/dashboard_v2; it resolved correctly on base and needs../../dashboard_v2. User-visible descriptions still direct readers to deleted paths indashboards/grafana/bqaa-dashboard.json:4,447,dashboards/grafana/bqaa-public-demo.json:46,240, and the error message atdashboards/grafana/run_local.py:111. Also update the old references in runner comments at lines 67/81, checker docstrings atscripts/check_grafana_queries_sync.py:536,675,783-784, and test comments/docstrings attests/test_check_grafana_queries_sync.py:219andtests/test_grafana_local_runner.py:178. This is incomplete migration under the requested review bar, not a query-semantics finding.
Verification: all 52 changed paths reviewed; 44 renames, including byte-identical copies of all 37 SQL files and both dashboard JSON files. The other five moved files match byte-for-byte after reversing only dashboards/grafana path substitutions. All four changed Python files have equal ASTs after normalizing the intended path edits. No SQL, panel behavior, runner control-flow, or CI execution/security change was found. The three Looker Studio references resolve to the new tree; the root README, package configuration, and remaining tracked runtime references need no additional executable-path update.
The standalone checker passes all 37 queries (19 interactive / 18 public demo); both targeted test modules pass: 80 passed on Python 3.13.5. Six scratch-copy negative probes correctly fail for interactive/public/template-query drift, missing SQL, missing dashboard, and an unmapped SQL file, even with a valid old grafana/ tree present. The moved runner's default --provision-only path works from another directory and writes under dashboards/grafana/.local/. No live Grafana or BigQuery run was performed. The test environment reports one unused asyncio_mode configuration warning. GitHub's Grafana-sync and general CI runs currently have action_required, so this is local verification, not a claim of passing remote CI.
PR #480 coordination: it remains OPEN/DRAFT at 08d42ac55e3dd8641d16502f0c1aaa24b01296e0. Its only shared changed file with #481 is .gitignore; a three-way merge reports a conflict in either order. Whichever lands second needs reconciliation. Once combined, #480's dashboards/README.md:10 must link to grafana/README.md, and its Streamlit references at app.py:210, models.py:17, and queries.py:86 need the new repository path. No shared-README textual conflict exists today because only #480 adds that file. This review does not approve #480 or change its draft state.
… path migrations - Retain legacy grafana/ credential ignore rules in .gitignore alongside new dashboards/grafana/ rules. - Fix relative link to dashboard_v2 in dashboards/grafana/README.md. - Update remaining grafana/ path references across dashboard JSON descriptions, local runner, sync checker docstrings, and tests.
caohy1988
left a comment
There was a problem hiding this comment.
REVIEW_VERDICT=REQUEST_CHANGES
P0=0 / P1=0 / P2=1
PRIOR_P1S_CLEARED=YES (2/2)
Fresh review of pinned HEAD e63e5ab61e178f6253d1c8ab98c45860cdb24d6e against GoogleCloudPlatform main 120da786c6f47122ac48ebeef5c8dfa9c7cd95b1, including the full migration and the delta from 47620b8.
P2 — Format the migrated path assignments so CI can pass. At scripts/check_grafana_queries_sync.py:14 and tests/test_check_grafana_queries_sync.py:38, the added directory component makes the assignments exceed the configured 80-column layout. Pyink 25.12.0, within CI's pyink>=24.3.0,<26 range, requires both assignments to be parenthesized and wrapped. This is an executable CI failure: .github/workflows/ci.yml:44-53 runs autoformat.sh and exits 1 when formatting changes scripts/ or tests/. Read-only reproduction:
pyink --check --diff --config pyproject.toml \
scripts/check_grafana_queries_sync.py \
tests/test_check_grafana_queries_sync.py \
tests/test_grafana_local_runner.pyResult: exit 1; two files would be reformatted. The same three files from the base revision pass with exit 0. Apply the formatter's two assignment wraps and rerun the check. This is a newly identified residual from the original migration, not a regression introduced by the repair commit.
Both prior P1s are closed:
- Legacy credential ignores:
.gitignore:38-45retains all three old rules alongside the new ones. Six harmless fixture paths covering JSON keys, datasource YAML and generated provisioning under both directory trees remain ignored and absent fromgit add --dry-run .; the two shipped dashboard JSON files remain tracked. - Documentation/help paths:
dashboards/grafana/README.md:5resolves to the existing rootdashboard_v2, and all previously reported JSON descriptions, runner help/error text, checker docstrings and test references use the new paths. All 19 relative file links in the moved Markdown resolve. Remaining old-looking slash references are intentional legacy ignore rules, Docker image names or/etc/grafanacontainer/system paths.
Verification: all 52 changed paths audited, including 44 moves; all 37 SQL files are byte-identical to base. Both parsed dashboard JSON objects and all four affected Python ASTs are unchanged after normalizing intended path substitutions. Grafana workflow filters and checker/test consumers target the new tree; package declarations are unchanged. The standalone checker passes 37 queries (19 interactive / 18 public demo), and the two targeted test modules pass 80 tests on Python 3.13.5. Seven scratch-fixture negative probes fail correctly for interactive/public/template drift, missing SQL, either missing dashboard and unmapped SQL, even with a valid legacy tree present. The runner's default --provision-only output lands in the new .local/ directory with datasource mode 0600.
Remote Grafana-sync and general CI runs currently report action_required, not success. No live Grafana/BigQuery run or package build was performed. The minimal test environment reports one unused asyncio_mode warning. No source fixes were applied.
PR #480 remains OPEN/DRAFT at 11b2624; its .gitignore still produces a three-way conflict with this head. Whichever PR lands second must reconcile it and update #480's Grafana references. That coordination item is separate from the single current-head P2 above.
| REPOSITORY_ROOT = Path(__file__).resolve().parents[1] | ||
| DASHBOARD_PATH = REPOSITORY_ROOT / "grafana" / "bqaa-dashboard.json" | ||
| QUERIES_DIRECTORY = REPOSITORY_ROOT / "grafana" / "queries" | ||
| DASHBOARD_PATH = REPOSITORY_ROOT / "dashboards" / "grafana" / "bqaa-dashboard.json" |
There was a problem hiding this comment.
P2: This migrated assignment and tests/test_check_grafana_queries_sync.py:38 need the wrapping required by the configured formatter. Pyink 25.12.0 --check --diff exits 1 on these two files; the base revision passes. CI runs autoformat.sh and fails when scripts/ or tests/ change (.github/workflows/ci.yml:44-53). Please apply the two assignment wraps so the format job can pass. Both prior P1 findings are cleared; full reproduction and verification are in the review body.
caohy1988
left a comment
There was a problem hiding this comment.
REVIEW_VERDICT=APPROVE
P0=0 / P1=0 / P2=0
PRIOR_P1S_CLEARED=YES (2/2)
PYINK_P2_CLOSED=YES
Re-reviewed e63e5ab61e178f6253d1c8ab98c45860cdb24d6e -> pinned HEAD a4b9464a12e27b86b0e09b66e6ab0613f01bf704 using the path-migration severity bar.
The entire delta is the two requested parenthesized assignment wraps. Both files have identical Python ASTs before and after; no behavior or path value changes.
The previous P2 is closed. Pyink 25.12.0 (within CI's configured range) exits 0 with "2 files would be left unchanged":
pyink --check --diff --config pyproject.toml \
scripts/check_grafana_queries_sync.py \
tests/test_check_grafana_queries_sync.pyBoth prior P1s remain closed:
- Legacy and migrated JSON-key, datasource-YAML and generated-provisioning paths remain ignored. Six harmless fixture files are excluded from
git add --dry-run .; the two shipped dashboards remain tracked. - The React link resolves to root
dashboard_v2, all 19 relative file links in the moved Markdown resolve, and the previously reported stale help/doc references remain corrected. Remaining legacy slash references are the intentional ignore rules, Docker image name and system/container provisioning paths.
Local verification: 80 targeted tests passed; standalone checker confirms 37 synchronized queries (19 interactive / 18 public demo); git diff --check passes. The minimal test environment emits one unused asyncio_mode configuration warning.
GitHub now reports successful format, Grafana-sync, Python 3.10-3.14 tests, browser smoke and package-build checks for this PR head. No source fixes or merge performed; the specified worktree remains clean at the pinned commit.
…gleCloudPlatform#481 alignment, and CI check and tests
There are no logic changes to any of the code, strictly updating path references.
This is a separate PR to manage the migration & consolidation of the Grafana Dashboard (grafana/) into a more manageable dashboards/ directory, including an update to references in CI workflow, sync script, tests, .gitignore, and cross-reference links and docs.
The vision is to create a clearly structured directory of supported dashboard options within dashboards/ for users to easily explore, install only the necessary dependencies, and deploy quickly.
The intention of this separate PR is to make the review and file changes more transparent and digestible and to ensure we catch any and all references to old paths.
Note: This will have some overlap with #480 and, depending on merge timeline, will require an update to the dashboards/README.md's Grafana reference in one of the two PRs