Skip to content

apis: allow debug.breakpoints.beforeSteps without onFailure - #10591

Open
pujitha24 wants to merge 1 commit into
tektoncd:mainfrom
pujitha24:auto/issue-9719
Open

apis: allow debug.breakpoints.beforeSteps without onFailure#10591
pujitha24 wants to merge 1 commit into
tektoncd:mainfrom
pujitha24:auto/issue-9719

Conversation

@pujitha24

Copy link
Copy Markdown

Changes

validateDebug in pkg/apis/pipeline/{v1,v1beta1}/taskrun_validation.go unconditionally
rejected any TaskRun with a non-nil debug.breakpoints unless breakpoints.onFailure was
also explicitly set to "enabled". onFailure and beforeSteps are independent breakpoint
types (onFailure pauses after a step fails, beforeSteps pauses before a named step for
inspection), so a TaskRun that only wanted beforeSteps was forced to also set
onFailure: enabled. Valid YAML like:

spec:
  debug:
    breakpoints:
      beforeSteps:
        - my-step

was rejected at admission with "onFailure breakpoint is empty, it is only allowed to be set as enabled".

This removes the OnFailure == "" rejection in both v1 and v1beta1 (the duplicated
validateDebug implementations are kept in lockstep). The remaining check - that a
non-empty onFailure must equal "enabled" - is unchanged, so onFailure is now optional
but still validated when set.

This is a narrower, separate concern from the volume-cleanup fix from a previous change to
this function (#7787): that fix gates debug-volume creation in pkg/pod/pod.go on
Debug.NeedsDebug(), which already treats a fully-empty Breakpoints{} (and a
Breakpoints without onFailure=="enabled") as a no-op independent of admission
validation, so this change does not reintroduce the redundant-volume behavior that earlier
fix addressed. A new test case locks in that a fully empty Breakpoints{} continues to
validate as a no-op.

User-visible behavior changes only in that a TaskRun using beforeSteps without
onFailure is now accepted instead of rejected at admission; no runtime/pod behavior
changes.

Validation performed:

  • go build ./...
  • go test ./pkg/apis/pipeline/v1/... ./pkg/apis/pipeline/v1beta1/... (all pass)
  • golangci-lint run --new-from-rev=HEAD ./pkg/apis/pipeline/v1/... ./pkg/apis/pipeline/v1beta1/...: 0 new issues (mirrors this repo's CI lint job, which lints only the diff against the merge base)
  • gofmt -l on the four changed files: clean
  • Failing-then-passing reproduction: stashing only the two production taskrun_validation.go changes (keeping the new test) makes the new "beforeSteps breakpoint without onFailure" case fail with the old "onFailure breakpoint is empty" error in both v1 and v1beta1; restoring the fix makes it pass.
  • Not run: no live cluster / e2e reproduction against a real TaskRun admission webhook - not required here since the defect and fix are fully exercised by the unit-test validation path above.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
  • Has Tests included if any functionality added or changed
  • pre-commit Passed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including functionality, content, code)
  • Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
  • Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
  • Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

**TaskRun**: `debug.breakpoints.beforeSteps` can now be set without also setting
`debug.breakpoints.onFailure`. Previously, a TaskRun with only `beforeSteps` configured
was rejected at admission requiring `onFailure: enabled` to also be set, even though the
two breakpoint types are independent.

Fixes #9719

Motivation:
validateDebug in pkg/apis/pipeline/{v1,v1beta1}/taskrun_validation.go
unconditionally rejected any TaskRun with a non-nil debug.breakpoints
unless breakpoints.onFailure was also explicitly set to "enabled".
onFailure and beforeSteps are independent breakpoint types (onFailure
pauses after a step fails, beforeSteps pauses before a named step for
inspection), so a TaskRun that only wants beforeSteps was forced to
also set onFailure: enabled. Valid YAML like:

  spec:
    debug:
      breakpoints:
        beforeSteps:
          - my-step

was rejected at admission with "onFailure breakpoint is empty, it is
only allowed to be set as enabled".

Approach:
Remove the `OnFailure == ""` rejection in both v1 and v1beta1 (the
duplicated validateDebug implementations are kept in lockstep). The
remaining check - that a non-empty onFailure must equal "enabled" -
is unchanged, so onFailure is now optional but still validated when
set. This is a narrower, separate concern from a prior fix to this
function (see tektoncd#7787) that
addressed redundant debug volumes appearing for an empty onFailure
value: that fix gates debug-volume creation in pkg/pod/pod.go on
Debug.NeedsDebug(), which already treats a fully-empty Breakpoints{}
(and a Breakpoints without onFailure=="enabled") as a no-op
independent of admission validation, so this change does not
reintroduce the redundant-volume behavior that earlier fix addressed.
A new test case locks in that a fully empty Breakpoints{} continues
to validate as a no-op.

User-visible behavior changes only in that a TaskRun using beforeSteps
without onFailure is now accepted instead of rejected at admission;
no runtime/pod behavior changes.

Validation:
- go build ./...
- go test ./pkg/apis/pipeline/v1/... ./pkg/apis/pipeline/v1beta1/...
  (all pass)
- golangci-lint run --new-from-rev=HEAD on both packages: 0 new issues
  (mirrors the repo's CI lint job, which lints only the diff against
  the merge base)
- gofmt -l on the four changed files: clean
- Confirmed the fix with a failing-then-passing reproduction: stashing
  only the two production taskrun_validation.go changes (keeping the
  new test) makes the new "beforeSteps breakpoint without onFailure"
  case fail with the old "onFailure breakpoint is empty" error in both
  v1 and v1beta1; restoring the fix makes it pass.

/kind bug

```release-note
**TaskRun**: `debug.breakpoints.beforeSteps` can now be set without
also setting `debug.breakpoints.onFailure`. Previously, a TaskRun with
only `beforeSteps` configured was rejected at admission requiring
`onFailure: enabled` to also be set, even though the two breakpoint
types are independent.
```

Report: tektoncd#9719
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Assisted-by: Claude Sonnet 5 (via Claude Code)
@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 13, 2026
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 13, 2026
@pujitha24

Copy link
Copy Markdown
Author

/assign @abayer

This is rebased and green — happy to address any feedback.

@tekton-robot

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from waveywaves after the PR has been reviewed.

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

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Debug validation bug: beforeSteps cannot be used without onFailure

4 participants