Skip to content

TRT-2737: Remove test_analysis_by_job_by_dates table and BQ loader#3826

Open
mstaeble wants to merge 1 commit into
openshift:mainfrom
mstaeble:worktree-trt-2737-phase2
Open

TRT-2737: Remove test_analysis_by_job_by_dates table and BQ loader#3826
mstaeble wants to merge 1 commit into
openshift:mainfrom
mstaeble:worktree-trt-2737-phase2

Conversation

@mstaeble

@mstaeble mstaeble commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Phase 2 of TRT-2737: now that all consumers have been rewritten to use test_daily_totals and test_cumulative_summaries (PR #3747), this PR removes the now-unused test_analysis_by_job_by_dates infrastructure:

  • Drop the TestAnalysisByJobByDate GORM model
  • Remove the BigQuery loader (loadDailyTestAnalysisByJob) and its supporting types/functions
  • Remove the ProwLoaderTestAnalysis BigQuery label constant
  • Remove partition management registration for the table
  • Remove the table from e2e partition tests
  • Add migration 000008 to explicitly drop the prow_test_analysis_by_variant_14d_view view, the test_analysis_by_job_by_dates table (including all partitions), and any detached partitions
  • Remove dead code: LoadProwJobCache and LoadTestCache (only callers were in the removed loader)
  • Update the plan_cache_mode=force_custom_plan comment to reflect the actual rationale
  • Update the baseline migration existence check to use prow_job_run_tests

Test plan

  • make lint passes clean
  • make test passes (15055 Go tests)
  • make e2e passes (migration 000008 runs successfully)
  • Down migration schema matches original definition from migration 000001

🤖 Generated with Claude Code

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: automatic mode

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 25, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 25, 2026

Copy link
Copy Markdown

@mstaeble: This pull request references TRT-2737 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

Phase 2 of TRT-2737: now that all consumers have been rewritten to use test_daily_totals and test_cumulative_summaries (PR #3747), this PR removes the now-unused test_analysis_by_job_by_dates infrastructure:

  • Drop the TestAnalysisByJobByDate GORM model
  • Remove the BigQuery loader (loadDailyTestAnalysisByJob) and its supporting types/functions
  • Remove the ProwLoaderTestAnalysis BigQuery label constant
  • Remove partition management registration for the table
  • Remove the table from e2e partition tests
  • Add migration 000008 to explicitly drop the prow_test_analysis_by_variant_14d_view view and the test_analysis_by_job_by_dates table (including all partitions)
  • Update the plan_cache_mode=force_custom_plan comment to reflect the actual rationale (GORM parameterized queries and partition pruning) rather than referencing the removed table
  • Update the baseline migration existence check to use prow_job_run_tests instead of the dropped table

Test plan

  • make lint passes clean
  • make test passes (15055 Go tests)
  • make e2e passes (migration 000008 runs successfully)
  • Down migration schema matches original definition from migration 000001

🤖 Generated with Claude Code

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci
openshift-ci Bot requested review from deepsm007 and sosiouxme July 25, 2026 13:39
@openshift-ci

openshift-ci Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mstaeble

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2026
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

The PR removes daily test-analysis-by-job loading and its exported data contracts, drops the legacy database table and view through migration, and excludes that table from partition lifecycle management and end-to-end partition checks.

Changes

Legacy test-analysis removal

Layer / File(s) Summary
Remove loader and data contracts
pkg/bigquery/bqlabel/labels.go, pkg/dataloader/prowloader/..., pkg/db/models/prow.go
The loader no longer imports daily test-analysis data, and related helpers, tests, query labels, and model types are removed.
Drop legacy database objects
pkg/db/migrations/..., pkg/db/migrate/migrate.go
Migration 000008 drops the legacy view and table, provides table recreation SQL for rollback, and updates legacy baseline detection.
Remove partition management
pkg/db/db.go, test/e2e/db/partitions/partitions_test.go
Partition lifecycle code and verification queries no longer include test_analysis_by_job_by_dates; planner documentation is updated.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • openshift/sippy#3747: Rewrites test-analysis queries to use test_daily_totals and test_cumulative_summaries while this PR removes the legacy table and loader.

Suggested labels: ready-for-human-review

Suggested reviewers: petr-muller, neisw

🚥 Pre-merge checks | ✅ 20 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Test Coverage For New Features ⚠️ Warning RunMigrations' new legacy-db baseline path has no targeted regression test; existing e2e tests only smoke-test migrations. Add a test that simulates a DB without schema_migrations but with prow_job_run_tests, and verify migration 000008 up/down schema (incl. the dropped view).
✅ Passed checks (20 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Go Error Handling ✅ Passed Added Go code wraps errors with fmt.Errorf, and the PR removes the old _ = row.Scan(...) ignore; no new panic or nil-deref patterns.
Sql Injection Prevention ✅ Passed The only changed SQL file is static DDL; the touched DB code uses placeholders or constants, and no new user-input SQL concatenation was added.
Excessive Css In React Should Use Styles ✅ Passed PASS: The PR only changes Go/SQL/tests; no React components or inline style objects are touched, so the CSS guidance is not applicable.
Single Responsibility And Clear Naming ✅ Passed PASS: the PR removes the broad test-analysis chunk and leaves focused names like TestDailyTotal/TestCumulativeSummary, PartitionedTables, and RunMigrations; no new generic APIs were introduced.
Feature Documentation ✅ Passed No docs/features page covers the removed table/loader path; the only feature doc is about job symptoms/labels and doesn’t need updating.
Stable And Deterministic Test Names ✅ Passed The changed test file uses only static t.Run titles; no Ginkgo-style titles or dynamic values appear in test names.
Test Structure And Quality ✅ Passed PASS: the changed test file is standard Go subtests, not Ginkgo; PR only narrows SQL filters and adds no new setup, timeout, or assertion-pattern issues.
Microshift Test Compatibility ✅ Passed No new Ginkgo e2e tests were added; the only changed e2e file is a plain Go DB partition test with no OpenShift API usage or MicroShift-sensitive features.
Single Node Openshift (Sno) Test Compatibility ✅ Passed PASS: The touched e2e test is a plain testing.T partition lifecycle test, not Ginkgo, and it contains no multi-node/HA assumptions.
Topology-Aware Scheduling Compatibility ✅ Passed PASS: The PR only removes DB/loader/migration/test code; no deployment manifests, controllers, or scheduling constraints were added or modified.
Ote Binary Stdout Contract ✅ Passed Touched files have no init/TestMain/BeforeSuite hooks and no stdout writers (fmt.Print/klog.SetOutput/log.SetOutput).
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No new Ginkgo e2e test bodies were added; the only e2e change is SQL filter tweaks in an existing standard Go test, with no IPv4 or external-network assumptions.
No-Weak-Crypto ✅ Passed Scanned all PR files; none contain MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret comparisons.
Container-Privileges ✅ Passed The PR touches only Go/SQL/manifest files; no K8s/container manifests were changed and no privileged/hostPID/hostNetwork/allowPrivilegeEscalation settings appear in the diff.
No-Sensitive-Data-In-Logs ✅ Passed PR diff adds no new logging of secrets/PII; changed lines are removals, comments, SQL, and one non-sensitive error string.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: removing the test_analysis_by_job_by_dates table and its BigQuery loader.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mstaeble

Copy link
Copy Markdown
Contributor Author

/hold

To let the changes soak in production before we destructively drop the table.

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 25, 2026
@mstaeble mstaeble changed the title TRT-2737: Remove test_analysis_by_job_by_dates table and BQ loader [WIP] TRT-2737: Remove test_analysis_by_job_by_dates table and BQ loader Jul 25, 2026
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 25, 2026
@mstaeble
mstaeble marked this pull request as draft July 25, 2026 13:42

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@pkg/db/migrations/000008_drop_test_analysis_by_job_by_dates.down.sql`:
- Around line 1-14: The down migration currently recreates only
test_analysis_by_job_by_dates; also restore
prow_test_analysis_by_variant_14d_view with the same definition it had before
migration 000008. Add the matching CREATE VIEW statement to the rollback, while
preserving the existing table recreation and ensuring the up migration still
removes the view when it is no longer part of the schema.

In `@test/e2e/db/partitions/partitions_test.go`:
- Line 42: Update every changed parent-table filter in the partition e2e checks,
including the WHERE clauses using parent.relname and c.relname, to include
test_daily_totals and test_cumulative_summaries alongside the existing
prow_job_run_tests and prow_job_run_test_outputs entries. Apply this
consistently to all referenced filter locations so coverage matches
PartitionedTables().
🪄 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: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 3b232386-aef7-4465-9b98-9c0741615a27

📥 Commits

Reviewing files that changed from the base of the PR and between 1c5aaf7 and aa357d7.

📒 Files selected for processing (10)
  • pkg/bigquery/bqlabel/labels.go
  • pkg/dataloader/prowloader/prow.go
  • pkg/dataloader/prowloader/prow_test.go
  • pkg/db/db.go
  • pkg/db/migrate/migrate.go
  • pkg/db/migrations/000008_drop_test_analysis_by_job_by_dates.down.sql
  • pkg/db/migrations/000008_drop_test_analysis_by_job_by_dates.up.sql
  • pkg/db/migrations/MANIFEST
  • pkg/db/models/prow.go
  • test/e2e/db/partitions/partitions_test.go
💤 Files with no reviewable changes (4)
  • pkg/db/models/prow.go
  • pkg/bigquery/bqlabel/labels.go
  • pkg/dataloader/prowloader/prow_test.go
  • pkg/dataloader/prowloader/prow.go

Comment thread test/e2e/db/partitions/partitions_test.go
Phase 2 of TRT-2737: now that all consumers have been rewritten to use
test_daily_totals and test_cumulative_summaries (PR openshift#3747), remove the
now-unused test_analysis_by_job_by_dates infrastructure:

- Drop the TestAnalysisByJobByDate GORM model
- Remove the BigQuery loader and its supporting types/functions
- Remove the ProwLoaderTestAnalysis BigQuery label constant
- Remove partition management registration for the table
- Remove the table from e2e partition tests
- Add migration 000008 to explicitly drop the dependent view and table
- Update the plan_cache_mode comment to reflect the actual rationale
- Update the baseline migration existence check to use prow_job_run_tests

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@mstaeble
mstaeble force-pushed the worktree-trt-2737-phase2 branch from aa357d7 to d999649 Compare July 25, 2026 15:01
@openshift-ci openshift-ci Bot added the ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review label Jul 25, 2026
@mstaeble
mstaeble marked this pull request as ready for review July 25, 2026 17:26
@mstaeble mstaeble changed the title [WIP] TRT-2737: Remove test_analysis_by_job_by_dates table and BQ loader TRT-2737: Remove test_analysis_by_job_by_dates table and BQ loader Jul 25, 2026
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 25, 2026
@openshift-ci
openshift-ci Bot requested review from dgoodwin and smg247 July 25, 2026 17:27
@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling required tests:
/test e2e

@openshift-ci

openshift-ci Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

@mstaeble: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants