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

Write command to fetch logs for a failed step in a new line #2303

Merged
merged 1 commit into from
Apr 17, 2020

Conversation

waveywaves
Copy link
Member

@waveywaves waveywaves commented Mar 29, 2020

Changes

It should be possible to copy/paste in one go
the command to fetch a failed test logs from
the status of a taskrun.
However, this is not possible as long, single-line
strings (i.e. does not contain "\n") are marshalled,
the yaml emitter forces line breaks at 80 characters.
Due to this the command is being divided into two,
which depriciates the user experience.

fixes #2221

Submitter Checklist

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

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.

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Mar 29, 2020
@tekton-robot tekton-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 29, 2020
@waveywaves
Copy link
Member Author

/assign @sbwsg
cc @afrittoli

@tekton-robot tekton-robot assigned ghost Mar 29, 2020
@waveywaves waveywaves force-pushed the 2221-logs-bug branch 2 times, most recently from f7fae53 to f311077 Compare March 29, 2020 17:15
@waveywaves
Copy link
Member Author

/test pull-tekton-pipeline-build-tests

1 similar comment
@waveywaves
Copy link
Member Author

/test pull-tekton-pipeline-build-tests

@ghost
Copy link

ghost commented Mar 30, 2020

Hey, could we do more with the commit message and PR description here? We typically try to include a lot more information than exists right now. From our guidelines:

All commit messages should follow these best practices, specifically:

Start with a subject line
Contain a body that explains why you're making the change you're making
Reference an issue number one exists, closing it if applicable (with text such as "Fixes #245" or "Closes #111")

Aim for 2 paragraphs in the body. Not sure what to put? Include:

What is the problem being solved?
Why is this the best approach?
What other approaches did you consider?
What side effects will this approach have?
What future work remains to be done?

Here is an example formatting for this commit:

Write command to fetch logs for a failed step in a new line

When a command fails Tekton includes a message with the kubectl command to check logs from the failing Task containers.  Unfortunately this message is limited in length by Tekton's YAML library and, for long container names, will be forced to wrap to a new line.  Copy-pasting the kubectl logs command will then not work.

This commit alleviates the problem by inserting a newline just before the kubectl logs command.  This will give some more characters for long container names to appear in a single line on the console.

pkg/pod/status.go Outdated Show resolved Hide resolved
pkg/pod/status.go Outdated Show resolved Hide resolved
@waveywaves waveywaves force-pushed the 2221-logs-bug branch 2 times, most recently from 98c1f3d to 4f3e0a3 Compare April 4, 2020 17:56
@waveywaves
Copy link
Member Author

@afrittoli @sbwsg I am having a hard time understanding what is wrong with the integration tests. Can you help out ?

@ghost
Copy link

ghost commented Apr 6, 2020

I0404 18:27:31.260] panic: test timed out after 20m0s

Looks like a timeout. Possibly unrelated to the code changes here. Retrying to see if it reproduces again.

/test pull-tekton-pipeline-integration-tests

@waveywaves
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

@waveywaves
Copy link
Member Author

@sbwsg It was a flake as you said

@ghost
Copy link

ghost commented Apr 14, 2020

Would be great to see a comment explaining the change but otherwise this looks good to me. Thanks!

/approve

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2020
It should be possible to copy/paste in one go
the command to fetch a failed test logs from
the status of a taskrun.
However, this is not possible as long, single-line
strings (i.e. does not contain "\n") are marshalled,
the yaml emitter forces line breaks at 80 characters.
Due to this the command is being divided into two,
which depriciates the user experience.
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.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg, 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 merged commit a734404 into tektoncd:master Apr 17, 2020
@afrittoli afrittoli added the kind/bug Categorizes issue or PR as related to a bug. label Apr 30, 2020
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. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The command to fetch the logs for a failed step can be broken in two
5 participants