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

Add StdoutConfig and StderrConfig to steps. #3103

Closed
wants to merge 10 commits into from

Conversation

chhsia0
Copy link
Contributor

@chhsia0 chhsia0 commented Aug 15, 2020

Changes

This PR implements Option 1 of TEP-0011.

Closes #2925.

Submitter Checklist

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

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

EXPERIMENTAL: Users can specify the TEKTON_STDOUT_FILE and TEKTON_STDERR_FILE environment variables in steps to capture steps' stdout and stderr to local files. This feature can be used to capture stdout and stderr into task results.

@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 15, 2020
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 15, 2020

CLA Check
The committers are authorized under a signed CLA.

@tekton-robot tekton-robot requested review from bobcatfish and a user August 15, 2020 20:56
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 15, 2020
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

The full list of commands accepted by this bot can be found 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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 15, 2020
@tekton-robot
Copy link
Collaborator

Hi @chhsia0. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@chhsia0
Copy link
Contributor Author

chhsia0 commented Aug 15, 2020

/kind feature

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 15, 2020
@jlpettersson
Copy link
Member

/ok-to-test

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 15, 2020
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
cmd/entrypoint/io.go Do not exist 86.3%
cmd/entrypoint/runner.go 81.0% 84.1% 3.1

@tekton-robot
Copy link
Collaborator

@chhsia0: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 16, 2020
@chhsia0 chhsia0 changed the title Added -stdout_path and -stderr_path flags to entrypoint. Added -stdout_file and -stderr_file flags to entrypoint. Aug 16, 2020
@vdemeester vdemeester added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Aug 17, 2020
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

@chhsia0 how do we envision being able to pass those flags from the "API" ?

cmd/entrypoint/runner.go Outdated Show resolved Hide resolved
@chhsia0
Copy link
Contributor Author

chhsia0 commented Aug 17, 2020

@vdemeester I'll create a TEP for passing the params :) Right now in its current state user would use env vars as a channel transparent to the Tekton controller. Sounds scary huh? ;)

@vdemeester
Copy link
Member

@vdemeester I'll create a TEP for passing the params :) Right now in its current state user would use env vars as a channel transparent to the Tekton controller. Sounds scary huh? ;)

😁 nothing is scary 😝 and as you said during the API WG, it allows you to more easily experiment with it without really commiting to it (aka see if it's useful and all).

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2020
@tekton-robot tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 6, 2020
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/pod/entrypoint.go 84.8% 85.9% 1.1

@chhsia0
Copy link
Contributor Author

chhsia0 commented Mar 10, 2021

/remove-lifecycle stale

@tekton-robot tekton-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 10, 2021
chhsia0 added 10 commits March 10, 2021 09:14
When `-stdout_path` and `-stderr_path` are set, the entrypoint binary will copy
stdout and stderr to the specified paths. This can be used to capture the stdout
or stderr of a step in task results.

The default values of these flags are obtained from the `TEKTON_STDOUT_PATH` and
`TEKTON_STDERR_PATH` environment variables, so users can specify these variables
in the step's environment to enable stdout/stderr capturing.
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2021
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
cmd/entrypoint/io.go Do not exist 86.8%
cmd/entrypoint/runner.go 84.0% 82.1% -1.9
pkg/pod/entrypoint.go 84.8% 85.9% 1.1

Base automatically changed from master to main March 11, 2021 15:28
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
cmd/entrypoint/io.go Do not exist 86.8%
cmd/entrypoint/runner.go 80.0% 80.4% 0.4
pkg/pod/entrypoint.go 84.8% 85.9% 1.1

@tekton-robot
Copy link
Collaborator

@chhsia0: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-tekton-pipeline-unit-tests a52c86f link /test tekton-pipeline-unit-tests
pull-tekton-pipeline-build-tests a52c86f link /test pull-tekton-pipeline-build-tests
pull-tekton-pipeline-integration-tests a52c86f link /test pull-tekton-pipeline-integration-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

1 similar comment
@tekton-robot
Copy link
Collaborator

@chhsia0: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-tekton-pipeline-unit-tests a52c86f link /test tekton-pipeline-unit-tests
pull-tekton-pipeline-build-tests a52c86f link /test pull-tekton-pipeline-build-tests
pull-tekton-pipeline-integration-tests a52c86f link /test pull-tekton-pipeline-integration-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@bobcatfish
Copy link
Collaborator

highly relevant: kubernetes/kubernetes#94892

@tekton-robot
Copy link
Collaborator

@chhsia0: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2021
@bobcatfish
Copy link
Collaborator

@chhsia0 I'm thinking maybe it makes sense at this point to close this PR for now, unless you're planning to pick it up again in the near future. We can always re-open.

@vdemeester
Copy link
Member

I think we can close, and I plan to carry this one soon 😇
/close

@tekton-robot
Copy link
Collaborator

@vdemeester: Closed this PR.

In response to this:

I think we can close, and I plan to carry this one soon 😇
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/feature Categorizes issue or PR as related to a new feature. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to extract results from a container's stdout
5 participants