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: add ccache pvc #654

Merged
merged 2 commits into from
Jul 27, 2023
Merged

feat: add ccache pvc #654

merged 2 commits into from
Jul 27, 2023

Conversation

jayl1e
Copy link
Contributor

@jayl1e jayl1e commented Jul 27, 2023

Why:

  • tiflash ccache need pvc to share cache
  • rust-toolchain and cargo can be cached

How:

  • add kustomization

Testing:

Signed-off-by: lijie <lijie@pingcap.com>
@ti-chi-bot ti-chi-bot bot requested review from purelind and wuhuizuo July 27, 2023 09:44
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jul 27, 2023

I have already done a preliminary review for you, and I hope to help you do a better job.
This pull request adds ccache pvc to share cache and caches rust-toolchain and cargo. The changes seem reasonable and well-documented. However, there are a few suggestions to consider:

  • It would be helpful to add more details about the testing, including what was tested and how it was tested.
  • The storage size of the pvc.yaml file may not be sufficient for future needs. It may be necessary to add more storage or modify the current storage size.
  • It is unclear whether the ceph-filesystem storageClassName is already defined in the cluster. If it is not, it may need to be created.

Overall, this pull request seems good. However, more information about testing and storage size may be needed.

@ti-chi-bot ti-chi-bot bot added the size/M label Jul 27, 2023
Signed-off-by: lijie <lijie@pingcap.com>
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jul 27, 2023

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

Review for PR: feat: add ccache pvc

Summary

This PR is adding a new feature to the project by adding a PVC (Persistent Volume Claim) for CCache. The PR includes the new PVC definition and a Kustomization file to apply the changes.

Potential Problems

  • There are no potential problems with this PR.

Fixing Suggestions

  • No fixing suggestions needed.

Overall Impression

The PR is adding a new feature to the project by adding a PVC for CCache. It is well documented and tested. The changes are simple and clear and do not affect the existing code. Therefore, I approve this PR.

Copy link
Collaborator

@wuhuizuo wuhuizuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jul 27, 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
Copy link
Contributor

ti-chi-bot bot commented Jul 27, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-07-27 10:02:35.435992545 +0000 UTC m=+435098.030523534: ☑️ agreed by wuhuizuo.

@ti-chi-bot ti-chi-bot bot added the approved label Jul 27, 2023
@ti-chi-bot ti-chi-bot bot merged commit b571839 into main Jul 27, 2023
3 checks passed
@ti-chi-bot ti-chi-bot bot deleted the pvc branch July 27, 2023 10:03
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.

2 participants