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(pipelines/pingcap/tidb/latest): enable coverage upload in job #2342

Merged
merged 2 commits into from
Aug 9, 2023

Conversation

Defined2014
Copy link
Contributor

@ti-chi-bot
Copy link

ti-chi-bot bot commented Aug 9, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
The pull request is adding a new feature to the existing codebase. It enables coverage upload in the job and uses the prow.uploadCoverageToCodecov function to upload the coverage data to Codecov. The new code is added in the stage("Checks") and post sections.

Potential problems:

  • It is not clear from the pull request description why this feature is needed or what problem it solves.
  • The pull request lacks any tests to show that the new feature is working as expected.
  • It is also unclear whether the necessary authentication is in place to upload coverage data to Codecov, and whether this authentication is secure.

Fixing suggestions:

  • The pull request should have a more detailed description to explain the need for the new feature, how it works, and how it benefits the project.
  • The pull request should include tests for the new feature to ensure that it is working as expected.
  • The authentication used to upload coverage data to Codecov should be secure, and the pull request should include documentation on how to set up and use this authentication.

@ti-chi-bot ti-chi-bot bot added the size/S label Aug 9, 2023
Co-authored-by: wuhuizuo <wuhuizuo@126.com>
@ti-chi-bot
Copy link

ti-chi-bot bot commented Aug 9, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
The pull request adds a new feature to the pipeline for the PingCAP TiDB project, which enables coverage upload in the job. Specifically, it adds an environment variable for the CODECOV_TOKEN and a new post-build step to upload coverage data to Codecov.

One potential problem is that the CODECOV_TOKEN is hardcoded in the script, which could lead to security vulnerabilities if the token is exposed. It might be better to use a secret management tool like Vault or store the token in an environment variable outside of the script.

Another suggestion would be to add some tests for the new feature to ensure that it works as expected and does not introduce any regressions.

Overall, the pull request looks good, but the potential security issue and lack of tests should be addressed before merging.

@wuhuizuo
Copy link
Collaborator

wuhuizuo commented Aug 9, 2023

/hold

It's need to test.

@wuhuizuo
Copy link
Collaborator

wuhuizuo commented Aug 9, 2023

/lgtm
/approve

@ti-chi-bot ti-chi-bot bot added the lgtm label Aug 9, 2023
@ti-chi-bot
Copy link

ti-chi-bot bot commented Aug 9, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-08-09 07:28:00.660409664 +0000 UTC m=+97645.209425646: ☑️ agreed by wuhuizuo.

@ti-chi-bot
Copy link

ti-chi-bot bot commented Aug 9, 2023

[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 Aug 9, 2023
@wuhuizuo
Copy link
Collaborator

wuhuizuo commented Aug 9, 2023

@wuhuizuo
Copy link
Collaborator

wuhuizuo commented Aug 9, 2023

/unhold

it's passed.

@ti-chi-bot ti-chi-bot bot merged commit 11ec7ec into PingCAP-QE:main Aug 9, 2023
@Defined2014 Defined2014 deleted the upload-cov branch August 9, 2023 08:46
@wuhuizuo
Copy link
Collaborator

wuhuizuo commented Aug 9, 2023

Ref: #2171

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants