Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make debug scripts comply with new /tekton/run #4416

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

waveywaves
Copy link
Member

@waveywaves waveywaves commented Dec 11, 2021

Changes

  • fix debug scripts (in /tekton/debug/scripts) to write entrypoint completion
    files to /tekton/run/<step-no.>/out and /tekton/run/<step-no.>/out.err
    instead of /tekton/run/<step-no.> and /tekton/run/<step-no.>.err.

Submitter Checklist

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

  • Docs included if any changes are user facing
  • Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Release notes block below has been filled in or deleted (only if no user facing changes)

fixes #4224

/kind bug

Release Notes

- Bug fixes
Debug scripts were unusable due to addition of the new /tekton/run behavior. This commit makes them usable again.

@tekton-robot tekton-robot added the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label Dec 11, 2021
@tekton-robot tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 11, 2021
@waveywaves waveywaves force-pushed the fix/issue-4224 branch 5 times, most recently from 05674ed to f6fd5fb Compare December 13, 2021 03:38
@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 13, 2021
- fix debug scripts (in /tekton/debug/scripts) to write entrypoint completion
files to /tekton/run/<step-no.>/out and /tekton/run/<step-no.>/out.err
instead of /tekton/run/<step-no.> and /tekton/run/<step-no.>.err.
@ghost
Copy link

ghost commented Dec 13, 2021

/test pull-tekton-pipeline-alpha-integration-tests

@ghost
Copy link

ghost commented Dec 13, 2021

I think this PR fixes #4224 ?

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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

The pull request process is described here

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 14, 2021
@waveywaves
Copy link
Member Author

/kind fix

@waveywaves
Copy link
Member Author

/kind bug

@tekton-robot tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 14, 2021
@waveywaves
Copy link
Member Author

@sbwsg I wasn't able to replicate the behavior for #4224 but I was able to find some inconsistencies while testing which I have fixed here. I am still linking this issue to 4224 though as a tracker issue for this PR.

@ghost
Copy link

ghost commented Dec 14, 2021

/lgtm

It might be worth adding an alpha e2e test at some point to exercise the debug scripts if possible so we can catch regressions if they're broken with future changes to the entrypoint / Step filesystem.

@tekton-robot tekton-robot assigned ghost Dec 14, 2021
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021
@ghost
Copy link

ghost commented Dec 14, 2021

/test pull-tekton-pipeline-alpha-integration-tests

@waveywaves
Copy link
Member Author

@sbwsg yeah, it would be helpful to have integration tests. I would have already written them if I knew how to architect them for debug.

There might be something very obvious that I am missing because of which I haven't already written them, because testing the scripts involve having a failing taskrun, 'exec'ing into the failed step pod and then checking and executing that script using exec. I am not sure how to achieve this as it is not as straightforward as checking the final state of a taskrun after it's executed. Testing breakpoints is a more involved process. We must discuss this on either an issue or is this something that might need a bigger change to e2e tests for tekton ?

@mattmoor
Copy link
Member

/test check-pr-has-kind-label

@tekton-robot tekton-robot merged commit 50300d9 into tektoncd:main Dec 14, 2021
@waveywaves
Copy link
Member Author

@sbwsg yeah, it would be helpful to have integration tests. I would have already written them if I knew how to architect them for debug.

There might be something very obvious that I am missing because of which I haven't already written them, because testing the scripts involve having a failing taskrun, 'exec'ing into the failed step pod and then checking and executing that script using exec. I am not sure how to achieve this as it is not as straightforward as checking the final state of a taskrun after it's executed. Testing breakpoints is a more involved process. We must discuss this on either an issue or is this something that might need a bigger change to e2e tests for tekton ?

Created #4426 to track this concern

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. 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.

/tekton/debug scripts absent after step has failed
4 participants