-
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
TEP-0127: Larger results using sidecar logs - parsing sidecar logs to extract results #5840
Conversation
/hold wait for #5834 to be merged |
The following is the coverage report on the affected files.
|
6dcc538
to
2828651
Compare
The following is the coverage report on the affected files.
|
2828651
to
3abbf19
Compare
The following is the coverage report on the affected files.
|
3abbf19
to
16c01b2
Compare
The following is the coverage report on the affected files.
|
16c01b2
to
e0ea018
Compare
The following is the coverage report on the affected files.
|
e0ea018
to
bc9dc18
Compare
The following is the coverage report on the affected files.
|
bc9dc18
to
c51ab2c
Compare
The following is the coverage report on the affected files.
|
c51ab2c
to
98343b1
Compare
The following is the coverage report on the affected files.
|
98343b1
to
e4c16da
Compare
f6edd91
to
c43998c
Compare
The following is the coverage report on the affected files.
|
c43998c
to
43302b4
Compare
The following is the coverage report on the affected files.
|
43302b4
to
86a6be5
Compare
The following is the coverage report on the affected files.
|
86a6be5
to
abc4b79
Compare
The following is the coverage report on the affected files.
|
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.
Mostly looks good to me - just a few small comments!
abc4b79
to
d556af3
Compare
The following is the coverage report on the affected files.
|
d556af3
to
08f61b0
Compare
The following is the coverage report on the affected files.
|
/retest |
The following is the coverage report on the affected files.
|
/approve Looks good to me! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abayer, jerop 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 |
… results This PR addresses a part of TEP-0127 - parsing logs to extract results into the task run CRD. The taskrun reconciler fetches the logs of the injected sidecar container sidecar-tekton-log-results. The logs are already structured and can be un-marshalled via json. The reconciler parses these logs and adds the results to the task run CRD. If the size of the result is greater than the max-result-size configured by the user then the TaskRun fails with reason TaskRunResultLargerThanAllowedLimit. `Note`: the e2e test only covers the success case of using sidecar logs to extract results. This is because the failure cases are already tested via unit tests. Before approving this PR, review tektoncd#5834
08f61b0
to
cc2f6af
Compare
The following is the coverage report on the affected files.
|
/retest |
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.
/lgtm
@@ -186,6 +186,8 @@ const ( | |||
TaskRunReasonsResultsVerificationFailed TaskRunReason = "TaskRunResultsVerificationFailed" | |||
// AwaitingTaskRunResults is the reason set when waiting upon `TaskRun` results and signatures to verify | |||
AwaitingTaskRunResults TaskRunReason = "AwaitingTaskRunResults" | |||
// TaskRunReasonResultLargerThanAllowedLimit is the reason set when one of the results exceeds its maximum allowed limit of 1 KB | |||
TaskRunReasonResultLargerThanAllowedLimit TaskRunReason = "TaskRunResultLargerThanAllowedLimit" |
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.
Thanks for the PR @chitrangpatel !
I was wondering if there is any reason this is kept in v1beta1 only? If not I can open up a PR to keep v1 and v1beta1 synced for the reconciler changes.
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 think thats because the reconciler currently uses v1beta1. This reason is only called there. Its ok to keep them both I sync I suppose. That way, when we update the reconciler to point to v1, it works.
Changes
This PR addresses a part of TEP-0127 - parsing logs to extract results into the task run CRD.
The taskrun reconciler fetches the logs of the injected sidecar container
sidecar-tekton-log-results
. The logs are already structured and can be un-marshalled via json. The reconciler parses these logs and adds the results to the task run CRD. If the size of the result is greater than themax-result-size
configured by the user then the TaskRun fails with reasonTaskRunResultLargerThanAllowedLimit
.Before approving this PR, review #5834
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes
/kind feature