chore(release): promote staging to production for v1.3.0-rc.1 - #892
Merged
Conversation
The Drive sync kept one manifest object keyed by Drive file id alone. Nothing in it recorded which database the rows went into. So an ingest into a local prod copy marked the workbook done everywhere, and the next run against staging skipped it. The skip looked exactly like a normal one. The file read as processed while staging had zero rows. POSTGRES_DB is the only value that already changes when .env moves between databases, so the manifest key now derives from it. I considered a separate ENVIRONMENT variable and dropped it. It can fall out of step with the POSTGRES_* block it describes, and then the bug comes back quietly. Recording the database inside one shared manifest looked tempting. It does not work. Entries are keyed by file id, so holding two databases means restructuring the key rather than adding a field. Every run would also read-modify-write the same object with no generation precondition, so two runs at once clobber each other, and one corrupt object forces a re-ingest everywhere. CHEMISTRY_INGEST_MANIFEST_PATH still overrides the derived key for forced re-ingests and old manifests. It is commented out in .env.example so a fresh copy gets the scoped default.
Per-database manifests tell you whether a workbook is done for the database in front of you. They cannot tell you whether it reached staging or production, and that gap is the one real argument for a single shared manifest. This command closes it by reading every per-database manifest and merging them for display. It writes nothing, so no two ingest runs ever touch the same object. It also separates databases whose manifest failed to parse from databases that hold no entry for a file. Collapsing those two would print "not ingested here" for a manifest I could not read, which is a louder claim than the evidence supports.
Rows with a blank SampleNumber bypassed the duplicate check. _sample_exists_for_wclab returns False when wclab_id is None, so every run appended the row again under a fresh lettered sample point. nma_wclab_id is nullable with no unique constraint, so the database did not catch it either. WCLab_ID is the only thing that identifies a re-ingest. Without one, nothing distinguishes a reload from a new sample, so prep_record now rejects the row and the file aborts, the same as a missing SamplePointID. The None guard in _sample_exists_for_wclab stays. A None would compare as IS NULL, match a legacy row, and skip a real sample. A workbook with a blank SampleNumber now fails instead of loading rows that duplicate on the next run. The lab has to fill the value in.
…ngest-manifest fix(chemistry-ingest): kas-fix-chemistry-ingest-manifest
…nches--staging--components--OcotilloAPI chore(staging): release 1.3.0-rc.1
Contributor
There was a problem hiding this comment.
Pull request overview
Promotes the current staging state to production for the v1.3.0-rc.1 checkpoint, bringing in recent chemistry-ingest reliability improvements and associated release-please bookkeeping.
Changes:
- Scope the chemistry ingest manifest path per
POSTGRES_DBand add a cross-databaseoco water-chemistry manifest-statuscommand. - Reject LIMS rows with missing/blank
SampleNumber(WCLab_ID) to prevent non-idempotent re-ingests; add tests and update runbook/docs. - Update staging release-please artifacts for
1.3.0-rc.1.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
services/chemistry_drive.py |
Implements per-database manifest keying and adds cross-database manifest aggregation for the new CLI status command. |
cli/cli.py |
Adds water-chemistry manifest-status CLI to display per-database ingest status without writing anything. |
services/chemistry_lims.py |
Treats missing/blank SampleNumber as a mapping error to avoid unrecognizable re-ingests. |
tests/test_chemistry_drive.py |
Adds tests for per-db manifest pathing, override precedence, and cross-db overview behavior. |
tests/test_chemistry_lims.py |
Adds tests ensuring missing SampleNumber aborts ingestion and writes nothing. |
docs/chemistry-ingestion-runbook.md |
Updates ingest behavior documentation for the new SampleNumber requirement. |
.env.example |
Updates guidance to avoid a shared manifest override by default. |
CHANGELOG-rc.md |
Adds the 1.3.0-rc.1 entry. |
.release-please-manifest.staging.json |
Bumps staging manifest version to 1.3.0-rc.1. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Coverage✅ 79.30% total — gate is 75%. Coverage for the Python files changed in this PR
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot Autofix pushed three commits straight to staging against PR #892. The last of them (cbd5a5a) re-added the per-file merge block but deleted `return overview`, so manifest_overview() built the overview and returned None. That broke `oco water-chemistry manifest-status` and failed test_manifest_overview_merges_databases and test_manifest_overview_separates_corrupt_manifest. The findings those commits were answering did not hold against the code as merged in #886: the merge block was already correctly scoped inside the `for db` loop, and the non-dict manifest guard was already present. The net effect of the autofix run was to remove a working return statement. Restore it. tests/test_chemistry_drive.py (17) and tests/test_chemistry_lims.py (30) pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ew-return fix(chemistry): restore the return in manifest_overview
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
cli/cli.py:1172
- The
--nameoption defaults toNone, but the type annotation isstr. This is inconsistent with the rest ofcli/cli.py(which usesstr | Nonefor optional string options) and makes the function signature/type hints misleading.
name: str = typer.Option(
None,
"--name",
help="Only show workbooks whose file name contains this text.",
),
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
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.
Second promotion of
stagingtoproductionfor the 1.3.0 release, picking upthe work merged to
stagingafter #888.Adds (via #886):
fix(chemistry): scope manifest per databasefeat(chemistry): add manifest-status commandfix(chemistry): reject rows with no SampleNumberPlus the
chore(staging): release 1.3.0-rc.1bookkeeping commit (#891), whoseown diff is staging-only (
.release-please-manifest.staging.json,CHANGELOG-rc.md) and which release-please rewrites on the next Release PR.Checkpoint: prerelease
v1.3.0-rc.1was cut from this staging state.Merging this updates the open Release PR #890 in place — still
1.3.0, withthese commits added to its changelog. Merge #890 after this one; that is what
tags
v1.3.0and deploys production.