Skip to content

fix: handle structured TaskRun deletion safely - #3367

Open
Debashich wants to merge 1 commit into
tektoncd:mainfrom
Debashich:fix/taskrun-delete-follow
Open

Debashich wants to merge 1 commit into
tektoncd:mainfrom
Debashich:fix/taskrun-delete-follow

Conversation

@Debashich

Copy link
Copy Markdown
Contributor

Changes

  • Keep structured JSON output clean by routing warnings to stderr.
  • Handle TaskRuns owned by running PipelineRuns without blocking deletion of other eligible TaskRuns.
  • Return an empty deleted array when no TaskRuns are deleted in structured output.
  • Add regression tests for output streams, mixed eligible/ineligible TaskRuns, and empty deletion results.
  • Regenerate the TaskRun deletion command documentation.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you review them:

See [the contribution guide](https://github.com/tektoncd/cli/blob/master/CONTRIBUTING.md) for more details.

Release Notes

Fix structured output handling for TaskRun deletion, keeping warnings off stdout and allowing eligible TaskRuns to be deleted when others are protected by running PipelineRuns.

Follow-up to #3086, addressing the review feedback on structured TaskRun deletion behavior.

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 22, 2026
@tekton-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign pratap0007 after the PR has been reviewed.
You can assign the PR to them by writing /assign @pratap0007 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 22, 2026
@Debashich

Copy link
Copy Markdown
Contributor Author

Hey @divyansh42 , I’ve opened a follow-up PR addressing the structured TaskRun deletion fixes as suggested. Whenever you get some time, could you please take a look? Thanks !

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The mixed-deletion regression case uses fixtures and assertions from different test suites, leaving the central behavior change unverified.

1 open issue
What changed in this PR

This follow-up to #3086 improves structured TaskRun deletion output while preserving safeguards for TaskRuns owned by running PipelineRuns.

Changes:

  • Routes warnings to stderr and skips protected TaskRuns without blocking eligible deletions.
  • Emits an empty deleted array when nothing is deleted and adds regression tests.
  • Updates command help and generated documentation with JSON examples and force requirements.
File Description
pkg/​cmd/​taskrun/​delete.go Adjusts structured output, confirmation handling, and protected deletion behavior.
pkg/​cmd/​taskrun/​delete_test.go Adds structured-output and deletion regression cases.
docs/​man/​man1/​tkn-taskrun-delete.1 Updates manpage examples and safety guidance.
docs/​cmd/​tkn_taskrun_delete.md Updates command reference examples and safety guidance.

Review effort: Balanced

Comment thread pkg/cmd/taskrun/delete_test.go Resolved
@Debashich

Copy link
Copy Markdown
Contributor Author

Hi @divyansh42, could you please rerun the failed E2E workflow when you get a chance? The failure appears unrelated to the changes in this PR. Thanks!

@divyansh42

Copy link
Copy Markdown
Member

/retest

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The v1beta1 test runner ignores assertStreams, so the all-protected case can pass without verifying valid JSON output containing an empty, non-null deleted array.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Low severity v1beta1 tests ignore stream assertions and never validate JSON output

pkg/​cmd/​taskrun/​delete_test.go:1462

assertStreams is never read by the v1beta1 test runner, so this case checks only the returned error and passes even if stdout contains a prompt or no JSON. Handle the flag with separate SetOut/SetErr buffers and direct Execute(), as in the v1 runner. Assert that the all-protected case emits valid JSON with an empty, non-null deleted array.

1 resolved since last review

Review effort: Balanced

@Debashich

Copy link
Copy Markdown
Contributor Author

Hi @divyansh42, I've addressed the remaining review suggestion and pushed the fix. The E2E workflow is now failing with a CI error, which seems unrelated to the changes in this PR. Could you please rerun the failed workflow when you get a chance? Thanks!

@divyansh42

Copy link
Copy Markdown
Member

/retest

@divyansh42 divyansh42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few improvements in the help text as well :
can we add  -o yaml examples, the "Using -o json or -o yaml skips the confirmation prompt" line, and a note that TaskRuns owned by running PipelineRuns need  --force ?

Also, Can we add a partial delete-failure case and one or two YAML cases

Comment thread pkg/cmd/taskrun/delete.go Resolved
@Debashich

Copy link
Copy Markdown
Contributor Author

@divyansh42 I have addressed the suggested improvements, PTAL when you get a chance, Thanks!

@divyansh42

Copy link
Copy Markdown
Member

@divyansh42 I have addressed the suggested improvements, PTAL when you get a chance, Thanks!

Thanks @Debashich for doing the changes.
I think one last pending change from my side would be to add one  -o yaml  example to the help text (for instance  tkn taskrun delete --all -n quux -o yaml )?
It is a small gap, since the prompt line mentions yaml but no example shows it. Then make generated should refresh the docs and man page

@divyansh42

Copy link
Copy Markdown
Member

Also, please squash the commits into one. So that it is ready for merge

@divyansh42

Copy link
Copy Markdown
Member

/cc @pratap0007

Signed-off-by: Debashich <debashishsinha555@gmail.com>
@Debashich
Debashich force-pushed the fix/taskrun-delete-follow branch from 4b6fab0 to bd9b2c4 Compare November 4, 2026 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants