Conversation
Catalog all deprecated commands, flags, legacy code paths, and server-side backward-compatibility shims that should be removed in a v3 release. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Claude finished @jbpros's task in 4m 35s —— View job Review of PR #745 — v3 Cleanup Plan
SummaryThis is a thorough and well-organized catalog of v3 cleanup work. The latest commit adds good new sections (richer exit codes, evidence→attestation terminology, command aliases review, approval commands). I cross-referenced the plan items against the actual codebase and left inline comments on specific issues. Here are the key findings: Deprecation gaps — items listed for removal that aren't formally deprecated yet:
These should either get deprecation warnings in a v2.x release first, or the plan should explicitly note they'll be removed without a deprecation period. Items correctly marked as deprecated (verified in code):
Fragile references — line numbers (138–153, 220–221) will drift; use code landmarks instead. Risk callout — the Minor suggestion — the evidence→attestation section mixes breaking changes (flag/help text) with non-breaking internal renames ( What's good
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| | Flag | Command | Replacement | File | | ||
| |------|---------|-------------|------| | ||
| | `--visibility` | `create flow` | Remove (org-level concern, not flow-level) | `cmd/kosli/createFlow.go` | |
There was a problem hiding this comment.
Nit: --visibility is not currently marked as deprecated in createFlow.go (no DeprecateFlags or MarkDeprecated call for it). Same for --template — it's used in the legacy code path but never formally deprecated. Worth noting this distinction: these flags need to be deprecated (with a warning in v2.x) before removal in v3, or the plan should clarify they'll be removed without a deprecation period.
TODO.md
Outdated
| - [ ] Remove the `--template` string-slice flag and the legacy code path in `createFlow.go` (lines 138–153) | ||
| - [ ] Only keep the `--template-file` / `--use-empty-template` path, which hits `PUT /api/v2/flows/<org>/template_file` |
There was a problem hiding this comment.
The line numbers referenced here (138–153) are approximate and will drift as the file changes. Consider referencing the code by its identifying characteristics instead (e.g., "the if o.TemplateFile == "" && !o.UseEmptyTemplate branch") so this plan stays accurate over time.
|
|
||
| - [ ] Remove legacy flow detection code (lines 220–221) that checks for "legacy_flow" and skips new attestation logic | ||
| - [ ] Note: the `legacy_flow` resolution type in the server (`src/model/policy_compliance_checker.py`) is **defined but never created** — it's dead code on both sides |
There was a problem hiding this comment.
Same fragile line-number reference here (lines 220–221). The actual code is a case "legacy_flow": branch in a switch statement — that description would be more durable.
| #### Rename `pipeline_id` → `flow_id` Throughout | ||
|
|
||
| - [ ] Rename `pipeline_id` to `flow_id` in artifact model (`src/model/artifact.py`), attestation model, trail model | ||
| - [ ] Remove `pipeline_name` → `flow_name` converter in FastAPI Pydantic models (`src/fastapi_app/models/artifacts.py`) | ||
| - [ ] Update database indexes in `src/documentdb/indexes.py` (many still reference `pipeline_id`) | ||
| - [ ] Requires a data migration for existing documents |
There was a problem hiding this comment.
This is the highest-risk item in the plan — a cross-cutting rename with a required data migration. Consider calling out an explicit sequencing constraint: the data migration must land and complete before any code referencing flow_id is deployed, or both field names need to be supported during a transition window. This deserves its own slice/epic rather than being a bullet in a larger list.
Add approval commands removal, flow creation simplification, richer exit codes, evidence→attestation terminology cleanup, and command alias review items. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| | `kosli report approval` | Attestation workflow | `cmd/kosli/reportApproval.go` | | ||
| | `kosli request approval` | Attestation workflow | `cmd/kosli/requestApproval.go` | | ||
| | `kosli get approval` | Attestation workflow | `cmd/kosli/getApproval.go` | | ||
| | `kosli list approvals` | Attestation workflow | `cmd/kosli/listApprovals.go` | |
There was a problem hiding this comment.
The approval commands (report approval, request approval, get approval, list approvals) are not currently marked as deprecated in their cobra command definitions — no Deprecated: field is set on any of them. This is different from report artifact and snapshot server which are formally deprecated.
The plan should note this: these commands need deprecation warnings added in a v2.x release first (so users get advance notice), or the plan should explicitly state they'll be removed without a deprecation period in v3.
|
|
||
| | Flag | Command | Replacement | File | | ||
| |------|---------|-------------|------| | ||
| | `--visibility` | `create flow` | Remove (org-level concern, not flow-level) | `cmd/kosli/createFlow.go` | |
There was a problem hiding this comment.
Same note as the previous review: --visibility is not formally deprecated — there's no DeprecateFlags or MarkDeprecated call for it in createFlow.go. Same for --template. Both are still actively registered flags.
Consider adding deprecation warnings for these in a v2.x release before removing them in v3, or document that they'll be removed without a deprecation period.
|
|
||
| ### Flow Creation Simplification | ||
|
|
||
| - [ ] Remove the `--template` string-slice flag and the legacy code path in `createFlow.go` (lines 138–153) |
There was a problem hiding this comment.
The line numbers referenced here (138–153) are approximate and will drift as the file changes. Consider referencing the code by its identifying characteristics instead (e.g., "the if o.TemplateFile == "" && !o.UseEmptyTemplate branch") so this plan stays accurate over time.
(Echoing the previous review's feedback since it still applies after the latest commit.)
|
|
||
| ### Legacy Flow Handling in `assertArtifact.go` | ||
|
|
||
| - [ ] Remove legacy flow detection code (lines 220–221) that checks for "legacy_flow" and skips new attestation logic |
There was a problem hiding this comment.
Same fragile line-number reference (lines 220–221). The actual code is a case "legacy_flow": branch in a switch statement — that description would be more durable.
(Echoing the previous review since it still applies.)
|
|
||
| - [ ] Rename `pipeline_id` to `flow_id` in artifact model (`src/model/artifact.py`), attestation model, trail model | ||
| - [ ] Remove `pipeline_name` → `flow_name` converter in FastAPI Pydantic models (`src/fastapi_app/models/artifacts.py`) | ||
| - [ ] Update database indexes in `src/documentdb/indexes.py` (many still reference `pipeline_id`) | ||
| - [ ] Requires a data migration for existing documents |
There was a problem hiding this comment.
This is the highest-risk item in the plan — a cross-cutting rename with a required data migration. Consider calling out an explicit sequencing constraint: the data migration must land and complete before any code referencing flow_id is deployed, or both field names need to be supported during a transition window. This deserves its own slice/epic rather than being a bullet in a larger list.
(Echoing the previous review since it still applies and is critical.)
| - [ ] `snykJsonResultsFileFlag` in `root.go` — "evidence vault" | ||
| - [ ] `snykSarifResultsFileFlag` in `root.go` — "evidence vault" | ||
| - [ ] `attachmentsFlag` in `root.go` — "evidence vault" | ||
| - [ ] Internal variable names: `evidencePath`, `evidencePaths` in attestation code (`attestation.go`, `pullrequest.go`, `attestCustom.go`, `attestGeneric.go`, `attestSnyk.go`, `attestSonar.go`) |
There was a problem hiding this comment.
Nit: renaming internal variable names (evidencePath, evidencePaths) is a good cleanup but isn't a breaking change. Consider separating this section into "breaking" (flag/help text changes visible to users) and "non-breaking" (internal renames) to help prioritize. Internal renames could land in v2.x without waiting for v3.
1 participant
Catalog all deprecated commands, flags, legacy code paths, and server-side backward-compatibility shims that should be removed in a v3 release.