-
Notifications
You must be signed in to change notification settings - Fork 9
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
- Phase 1: Static analysis (build, vet, mod tidy) — all clean
- Phase 2: Read and review the TODO.md changes
- Phase 3: Cross-reference plan items against codebase
- Phase 4: Post review feedback
Summary
This 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:
--visibilityand--templateincreateFlow.go— noDeprecateFlags/MarkDeprecatedcalls- All four approval commands (
report,request,get,list) — noDeprecated:field set on any of them
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):
--registry-providerinflags.go✓--clusterand--service-nameinsnapshotECS.go✓--function-nameand--function-versioninsnapshotLambda.go✓-eshorthand infingerprint.goandsnapshotServer.go✓--require-provenanceincreateEnvironment.go✓report artifactandsnapshot servercommands ✓
Fragile references — line numbers (138–153, 220–221) will drift; use code landmarks instead.
Risk callout — the pipeline_id → flow_id rename (lines 117–121) is a cross-cutting change with a data migration dependency. It deserves its own epic with explicit sequencing constraints.
Minor suggestion — the evidence→attestation section mixes breaking changes (flag/help text) with non-breaking internal renames (evidencePath variables). The internal renames could land in v2.x without waiting for v3.
What's good
- Comprehensive coverage of both CLI and server-side cleanup in one place
- Clear table format for deprecated commands and flags with file references
- Good cross-referencing between CLI and server items (e.g., flow creation endpoint appears in both sections)
- Linking to the tracking issue (kosli-dev/server#5132) and related issues/PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Catalog all deprecated commands, flags, legacy code paths, and server-side backward-compatibility shims that should be removed in a v3 release.