feat(storage): apply_deployments table + CRUD + tasks.apply_deployment_id (foundation)#205
Open
Kiran01bm wants to merge 3 commits into
Open
feat(storage): apply_deployments table + CRUD + tasks.apply_deployment_id (foundation)#205Kiran01bm wants to merge 3 commits into
Kiran01bm wants to merge 3 commits into
Conversation
…t_id (foundation)
Lay down the storage foundation for the multi-deployment apply data
model (WS-MD-F = MD-1 + MD-2 + MD-4 bundled). Pure additive, zero
behaviour change, no caller reads or writes any of the new artifacts
yet — those land in MD-3 (config) and MD-5/MD-6 (write path, scheduler).
MD-1 — new `apply_deployments` table
One child row per (apply, deployment). Unique on (apply_id, deployment).
4-state machine: pending / in_progress / completed / failed. Acts as
the per-deployment slice that MD-5 will dual-write into and MD-6 will
move the active-apply lock onto.
MD-2 — Go model + ApplyDeploymentStore CRUD
- storage.ApplyDeployment struct (pkg/storage/types.go).
- state.ApplyDeployment + IsApplyDeploymentTerminal (pkg/state/).
- ApplyDeploymentStore interface (pkg/storage/storage.go):
Insert / Get / GetByApplyDeployment / ListByApply / UpdateState /
MarkStarted / MarkCompleted / MarkFailed / DeleteByApply.
- MySQL impl (pkg/storage/mysqlstore/apply_deployments.go); unique-key
conflict on (apply_id, deployment) returns ErrApplyDeploymentExists.
- Wired into mysqlstore.Storage; mockStorage stub added in the api
package tests so the interface change stays compile-clean.
- 20 integration tests (pkg/storage/mysqlstore/apply_deployments_test.go):
insert/get/list, isolation across applies, unique constraint,
state transitions (with started_at COALESCE), MarkFailed, DeleteByApply,
not-found and DB-error paths.
MD-4 — tasks.apply_deployment_id nullable FK
New nullable column + idx_apply_deployment_id on `tasks`. Intentionally
not added to `taskColumns`, so no existing Task read/write path scans
or writes it — MD-6 turns the write path on once the scheduler claims
per apply_deployments row.
Test utility: pkg/testutil/container.go grows a ColumnExists helper
mirroring TableExists.
TestEnsureSchema asserts both `apply_deployments` table and
`tasks.apply_deployment_id` column land via EnsureSchema.
Background and full plan:
WS-MD implementation plan (internal) lists this PR as MD-F and the
remaining sequence (MD-3, MD-5..MD-11). Approach was chosen in the
Slack thread on PR #194; the picked design is the
"Multi-Deployment Apply (apply_deployments join table)" model — keeps
apply_id as the user-facing handle, pushes per-deployment state +
locking down into the new child rows.
There was a problem hiding this comment.
Pull request overview
This PR lays the storage/schema foundation for the Multi-Deployment Apply data model by introducing an apply_deployments child table plus a new storage interface/implementation for CRUD and state transitions, and by adding a nullable tasks.apply_deployment_id column for future wiring.
Changes:
- Add MySQL DDL for new
apply_deploymentstable and extendtaskswith nullableapply_deployment_id+ index. - Introduce
storage.ApplyDeployment,state.ApplyDeploymentconstants, andApplyDeploymentStoreinterface + MySQL implementation. - Add integration coverage for the new store and EnsureSchema assertions (including a new
testutil.ColumnExistshelper).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/testutil/container.go | Adds ColumnExists helper for schema verification tests. |
| pkg/storage/types.go | Introduces storage.ApplyDeployment struct type. |
| pkg/storage/storage.go | Extends Storage with ApplyDeployments() and adds ApplyDeploymentStore interface. |
| pkg/storage/mysqlstore/storage.go | Wires MySQL storage to provide an ApplyDeploymentStore. |
| pkg/storage/mysqlstore/apply_deployments.go | Implements MySQL CRUD + state transition operations for apply_deployments. |
| pkg/storage/mysqlstore/apply_deployments_test.go | Adds integration tests covering store behavior and error cases. |
| pkg/storage/errors.go | Adds typed storage errors for apply-deployment not found/exists. |
| pkg/state/apply_deployment.go | Adds apply-deployment state constants + terminal-state helper. |
| pkg/schema/schema.go | Updates schema package table list comment to include apply_deployments. |
| pkg/schema/mysql/tasks.sql | Adds nullable apply_deployment_id column + index to tasks. |
| pkg/schema/mysql/apply_deployments.sql | Adds DDL for apply_deployments table and indexes. |
| pkg/api/handlers_test.go | Updates mock storage for new interface method; uses slices.Backward for reverse scan. |
| pkg/api/ensure_schema_integration_test.go | Ensures new table exists and asserts tasks.apply_deployment_id column is present. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review on PR #205. - storage.ApplyDeploymentStore.UpdateState doc no longer implies it writes started_at/completed_at; directs callers to Mark* for timestamped transitions. - mysqlstore.applyDeploymentStore.UpdateState and MarkStarted no longer surface ErrApplyDeploymentNotFound on idempotent retries. MySQL's default RowsAffected counts changed rows (not matched), so re-applying the same state, or re-issuing MarkStarted on an already-started row (COALESCE), reports 0 affected. New checkUpdatedOrExists helper disambiguates via SELECT EXISTS. - Add regression tests covering both idempotent paths.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2223aa164a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
aparajon
reviewed
Jun 1, 2026
aparajon
reviewed
Jun 1, 2026
Comment on lines
+8
to
+11
| // the lifecycle of a single deployment target (one Tern endpoint, one lock), | ||
| // so the engine-specific intermediate states (waiting_for_deploy, | ||
| // waiting_for_cutover, etc.) do not apply at this layer. Those still live on | ||
| // the apply / task it spawned. |
Collaborator
There was a problem hiding this comment.
I think each apply_deployment will need to have the same apply states 😅 . Some use-cases:
- It should be possible to cutover one
apply_deploymentwhile others are still waiting, which is needed for operator controlled rollouts (e.g. test in one deployment before rolling out to others progressively) - Triaging: operators should be able to independent track each rollout, and have the ability to remediate in the case that the scheduler can't auto-retry
This means apply state will need to be derived from apply_deployment states, similar to how apply is derived from task state today
Collaborator
Author
There was a problem hiding this comment.
thats a good point - i'll ensure that the apply_deployment / apply_operation carries the full apply state machine
… state machine Address PR #205 review feedback (Slack thread + inline comments): - Rename table, struct, store, errors, FK column, indexes, tests, and filenames to apply_operations. Each row carries both a deployment and a target, so "apply operation" matches the unit-of-work semantics (companion rename scheduler → operator lands in MD-6 where the claim loop actually moves). - Rename misleading GetByApplyOperation(applyID, deployment) helper to GetByApplyAndDeployment to match its parameter shape. - Expand state.ApplyOperation to mirror state.Apply exactly — same 12 canonical states plus the PlanetScale-specific lifecycle phases. Per-row cutover / stop / revert / triage now share the apply state vocabulary, and the upcoming applies.state derivation can reuse DeriveApplyState() verbatim instead of growing a parallel aggregator. MarkStarted now writes state=running (not in_progress). - Fix MarkCompleted / MarkFailed same-second idempotency: route both through checkUpdatedOrExists and switch completed_at to COALESCE(completed_at, NOW()) so a retried no-op write on an already- terminal row no longer returns ErrApplyOperationNotFound. Adds TestApplyOperationStore_MarkCompleted_Idempotent and TestApplyOperationStore_MarkFailed_Idempotent regression tests. - Drop internal MD-* workstream tracking labels from PR-visible comments (they're sequencing labels, not stable upstream vocabulary). Verified: go build, go build -tags integration, go vet, go vet -tags integration, and the pkg/state / pkg/storage / pkg/schema / pkg/api unit suites all pass. Testcontainers integration tests for the new store will run on CI (Docker Desktop API mismatch locally).
aparajon
reviewed
Jun 1, 2026
|
|
||
| // ErrApplyOperationNotFound is returned when an apply_operations child | ||
| // row does not exist for the given lookup key. | ||
| ErrApplyOperationNotFound = errors.New("apply deployment not found") |
Collaborator
There was a problem hiding this comment.
Should be apply operation
aparajon
reviewed
Jun 1, 2026
|
|
||
| // ErrApplyOperationExists is returned when an apply_operations row for | ||
| // (apply_id, deployment) is being inserted but already exists. | ||
| ErrApplyOperationExists = errors.New("apply deployment already exists") |
Collaborator
There was a problem hiding this comment.
Should be apply operation
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.
What
Storage foundation for the Multi-Deployment Apply model. Bundles MD-1 + MD-2 + MD-4 (WS-MD-F).
apply_deploymentstable: one row per(apply, deployment), unique on(apply_id, deployment), statespending/in_progress/completed/failed, index(deployment, state).storage.ApplyDeployment+state.ApplyDeployment+ApplyDeploymentStoreinterface (Insert/Get/List/UpdateState/MarkStarted/MarkCompleted/MarkFailed/DeleteByApply) + MySQL impl + 20 integration tests.tasks.apply_deployment_id+ index. Not added totaskColumnsyet; wired up in MD-6.testutil.ColumnExistshelper;TestEnsureSchemaasserts new table + column.Why
Per-deployment apply state needs its own row so the active-apply lock and scheduler can key on
(deployment, state)instead of overloadingapplies. This PR lands the schema + CRUD only — pure additive, zero behaviour change, no callers touch the new artifacts yet.Chosen design: 03-multi-deployment-apply-model.md. PR #194 re-scoped to stage A only; B–E superseded by WS-MD.
Bundled because MD-1 (table) is unusable without MD-2 (store), and MD-4 shares the same migration window; splitting would produce three PRs that all need to land before anything else proceeds.
Follow-ups
MD-3 (config), MD-5 (dual-write), MD-6 (scheduler + lock move), MD-7..11.
Validation
go build,go vet, unit tests ✅ (incl.-tags integration).TestApplyDeploymentStoreintegration tests not run locally (Docker Desktop / Testcontainers API mismatch); CI will exercise.