Repository navigation
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR adds -o/--output json support to tkn taskrun delete, enabling machine-readable output of deleted TaskRuns and extending the shared pkg/deleter helper with accessors for successful deletions (plus unit tests).
Changes:
- Added JSON output path for
taskrun delete, and suppressed interactive confirmation when JSON is requested. - Extended
pkg/deleterwith accessors for successful deletes / related deletes. - Added unit tests for the new deleter accessors and JSON output for
taskrun delete.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| pkg/deleter/deleter.go | Adds accessors for successful deletes and successful related deletes. |
| pkg/deleter/deleter_test.go | Adds unit tests validating the new deleter accessors. |
| pkg/cmd/taskrun/delete.go | Implements JSON output mode and adjusts delete flow to suppress prompts in JSON mode. |
| pkg/cmd/taskrun/delete_test.go | Adds tests asserting JSON output for taskrun delete. |
Suppressed comments (1)
pkg/deleter/deleter.go:113
- SuccessfulRelatedDeletes also returns the backing slice directly, which allows external mutation of Deleter state. Returning a copy prevents subtle bugs when callers append/modify the returned slice.
func (d *Deleter) SuccessfulRelatedDeletes() []string {
return d.successfulRelatedDeletes
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (5)
pkg/cmd/taskrun/delete.go:135
- JSON mode skips
CheckOptionsentirely, but that method performs validation before prompting. As a result,-o jsonaccepts invalid destructive combinations such as named TaskRuns with--all/--keep, and even an invocation with neither names nor--allsucceeds with an empty result. Keep the validation and suppress only the confirmation prompt.
if output != "json" {
if err := opts.CheckOptions(s, availableTrs, p.Namespace()); err != nil {
return err
}
}
pkg/cmd/taskrun/delete.go:190
- With
--force -o json, a TaskRun owned by a running PipelineRun writes this warning to stdout before the JSON document, making stdout invalid JSON. Send the warning to stderr so stdout remains machine-readable.
if !prFinished && !opts.ForceDelete && output != "json" {
pkg/cmd/taskrun/delete.go:240
- This returns success immediately after encoding and drops any deletion errors accumulated by
Deleter. A delete that fails after lookup (for example, due to a race or API error) is therefore reported as a successful command with only the failed name omitted; encode the result, then returnd.Errors().
return json.NewEncoder(s.Out).Encode(result)
pkg/cmd/taskrun/delete.go:233
- The JSON branch is unreachable for the two no-op
--task --keepcases above because they return after printing human-readable status text. For example, when--keepequals the number of matching TaskRuns,-o jsondoes not produce JSON at all. Route those early exits through the JSON renderer and return an emptydeletedarray.
if output == "json" {
pkg/cmd/taskrun/delete.go:237
- The
--taskdeletion path records deleted TaskRun names insuccessfulRelatedDeletes, so this field becomesnulleven when TaskRuns were successfully deleted. Include both direct and related successes in the JSON result.
Deleted: d.SuccessfulDeletes(),
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
pkg/cmd/taskrun/delete.go:243
- In
-o jsonmode this function currently returns after encoding JSON, so any accumulated deletion errors fromd.Errors()are dropped (command can exit 0 even when deletes failed). Also, some early returns in the--task/--keeppaths print human output and bypass JSON entirely, and when nothing is deleted the current slice construction can encode asnullinstead of[]. Consider (1) emitting JSON for the early-return paths, (2) always encoding an empty array when nothing was deleted, and (3) returningd.Errors()after successfully writing JSON.
if output == "json" {
result := struct {
Deleted []string `json:"deleted"`
}{
Deleted: append(append([]string(nil), d.SuccessfulRelatedDeletes()...), d.SuccessfulDeletes()...),
57de078 to
019d0a0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
pkg/cmd/taskrun/delete.go:246
- When neither accessor contains an item, these nested appends preserve a nil slice, so
encoding/jsonemits{"deleted":null}rather than the{"deleted":[]}contract asserted by the new no-op test. Initialize the accumulation with a non-nil empty slice so no-op deletions still produce an array.
Deleted: append(append([]string(nil), d.SuccessfulRelatedDeletes()...), d.SuccessfulDeletes()...),
|
@Debashich do you have any updates on this PR? |
019d0a0 to
c411e88
Compare
|
@Debashich could you please add release notes in the PR description and also add a sample output. |
|
/retest |
There was a problem hiding this comment.
🟡 Changes recommended
The CLI advertises several output formats while the implementation accepts only JSON.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
8e3b4f6 to
2774b93
Compare
|
@divyansh42 Quick update on what happened after your requested changes: I implemented all 3 review changes, added the release note + sample output, removed the generic output flags so only json is advertised, ran gofmt, regenerated the docs, and pushed the updated commit. During testing, one existing TaskRun delete unit test was failing due to a fixture/expected-output mismatch. I reproduced the same failure on the base branch, so I confirmed it wasn't caused by this PR, then fixed the test fixture and amended the commit. After the latest push, build, lint, unit tests, generated code, multi-arch, CodeQL and k8s-plus-one E2E are passing. The only remaining failure is k8s-oldest E2E. I checked the full E2E logs, and the failure is actually TestPipelineInteractiveStartE2E, which times out after 10 minutes in the go-expect interactive test path. The PipelineRun itself starts successfully before the test hangs. The TaskRun/Pipeline deletion E2Es are passing. So I haven't changed anything in the E2E tests/infrastructure since the remaining failure appears unrelated to this PR. |
|
/retest |
|
@Debashich Could you please add support for the |
|
@Debashich a recent PR #3318 has been merged, which covers other commands. |
2774b93 to
7daab8a
Compare
Signed-off-by: Debashich <debashishsinha555@gmail.com>
2ada9c9 to
7c8789b
Compare
|
@divyansh42 @pratap0007 I've made the requested changes, added validation for unsupported output formats, and updated the docs for JSON/YAML output. Everything is pushed now. Could you take another look when you get a chance? Thanks! |
Closes #2850
Changes
Add
--output json/yaml(-o) structured output support totkn taskrun delete.When specified, the command returns machine-readable output in JSON or YAML format. Using
-o jsonor-o yamlskips the interactive confirmation prompt, allowing non-interactive usage.Example:
Submitter Checklist
make check.make generated.See [the contribution guide](https://github.com/tektoncd/cli/blob/master/CONTRIBUTING.md) for more details.
Release Notes