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

feat(cloudevents-server): append tail logs for failed steps #132

Merged
merged 2 commits into from
Apr 16, 2024

Conversation

wuhuizuo
Copy link
Contributor

Signed-off-by: wuhuizuo wuhuizuo@126.com

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Copy link

ti-chi-bot bot commented Apr 16, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

The Pull Request introduces a new feature to the cloudevents-server where it appends tail logs for failed steps in the CI/CD pipeline.

Key Changes:

  1. The Tekton struct in config.go has been extended to include a new field: FailedStepTailLines.
  2. The handler.go file has been modified to include functionality for handling tail logs for failed steps.
  3. The handler_pipelinerun.go and handler_taskrun.go files have been modified to extract information from event logs and handle cases where no receivers are found.
  4. The lark.go file has been modified to include functions for composing and sending Lark messages.
  5. The tekton-run-notify.yaml.tmpl file has been modified to include logs in the notification message.
  6. The types.go file has been modified to include a new type stepInfo to store step related information including logs.
  7. The newly added function getStepLog in handler.go retrieves the step logs.

Potential Problems:

  1. There is no error handling for the function getStepLog. It can potentially fail if there is a problem with the HTTP request or if the response body cannot be read.
  2. The logs fetched from getStepLog are dumped into the message directly without any formatting or checks. If the log contains sensitive information or is too large, it may cause issues.
  3. The function getStepLog uses a GET request to fetch logs. If the logs are large, this might not be the best approach.
  4. There are no tests included in this PR to verify the new feature.

Suggestions for Fixes:

  1. Add error handling for the getStepLog function to ensure that errors are caught and handled appropriately.
  2. Consider sanitizing or formatting the logs before adding them to the message.
  3. Consider using a POST request or streaming the logs if they are expected to be large.
  4. Include tests to verify that the feature works as expected and does not break existing functionality.

@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Apr 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

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

@ti-chi-bot ti-chi-bot bot added the approved label Apr 16, 2024
@ti-chi-bot ti-chi-bot bot merged commit e41b124 into main Apr 16, 2024
3 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feature/add-tail-logs-for-failed-step branch April 16, 2024 09:31
ti-chi-bot bot pushed a commit to PingCAP-QE/ee-ops that referenced this pull request Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant