-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Print all CRD as YAML on test failure #182
Print all CRD as YAML on test failure #182
Conversation
cb97fa8
to
b321041
Compare
/ok-to-test |
if the test is purposely failing, we would not be able to merge the PR |
A sample of the output in case of a failing test can be found here: https://gubernator.knative.dev/build/knative-prow/pr-logs/pull/knative_build-pipeline/182/pull-knative-build-pipeline-integration-tests/1055088374320205825/ we will eventually have to make the test pass to be able to merge |
@pivotal-nader-ziada the purpose of making the test fail was to showcase the output and get feedback on it. If there is no additional output desired, I can update the other existing steps and fix the test to make the PR mergeable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have a couple comments about potentially refactoring the code a bit but very happy with getting this info dumped on test failure! i think this will be super useful :D
test/pipelinerun_test.go
Outdated
for _, i := range trs.Items { | ||
printOrAdd("TaskRun", i.Name, i) | ||
} | ||
logger.Info(string(output)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about making this a bit more compact with something like a for loop that iterates over a bunch of list
functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately all of the List
functions are different types, and there is no common interface for List
ing between the various clients, so I wasn't able to find a way to shorten this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kk, thanks for looking!
596f37a
to
6f8a835
Compare
I like the output now without the logs, the CRDs will be helpful in debugging |
I'm not sure why https://gubernator.knative.dev/build/knative-prow/pr-logs/pull/knative_build-pipeline/182/pull-knative-build-pipeline-integration-tests/1056783921334718466/ failed, it looks related to the test environment? |
it was a timeout, lets give it another shot and see if its a flakey test /test pull-knative-build-pipeline-integration-tests |
This will cause the `teardown` function to dump all Knative CRD objects for the currently configured namepsace to YAML. This will make debugging failed builds easier. Fixes tektoncd#145.
6f8a835
to
28780d2
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pivotal-nader-ziada, tanner-bruce 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 |
This code will dump out all build-pipeline CRDs when a test fails to
make debugging easier.
Currently purposely makes the PipelineRun test fail.
Fixes #145.