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 tools wget&hostname&free to tikv&tiflash ci images #472

Conversation

purelind
Copy link
Contributor

add tools wget&hostname&free to tikv&tiflash ci images.

@ti-chi-bot ti-chi-bot bot requested a review from wuhuizuo October 23, 2024 06:23
Copy link

ti-chi-bot bot commented Oct 23, 2024

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

Based on the pull request title and description, the author has added the tools wget, hostname, and free to the Tikv and Tiflash CI images. The changes include installing these tools and creating symbolic links to python2 and pip2.

One potential problem is the addition of the procps-ng package, which may conflict with other packages or have unintended consequences. Additionally, the changes to the symbolic links may cause compatibility issues with other scripts or tools that rely on python and pip.

To fix these issues, it would be best to test the changes thoroughly and ensure that there are no conflicts or compatibility issues. The author could also provide more information on why procps-ng was added and whether it is necessary for the CI images. Finally, it may be helpful to add comments in the Dockerfile explaining the changes and any potential issues to make it easier for future contributors to understand the changes.

@ti-chi-bot ti-chi-bot bot added the size/XS label Oct 23, 2024
Copy link

ti-chi-bot bot commented Oct 23, 2024

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

Based on the Pull Request, the key changes include adding wget, hostname, and free tools to Tikv and Tiflash CI images. Additionally, the following dependencies are also added: procps-ng and python27.

One potential problem is that the python27 package is already installed in both Dockerfiles. It is redundant to install it again.

A suggestion to fix this would be to remove the installation of python27 in both Dockerfiles since it is already installed. Another suggestion would be to add comments to the Dockerfile explaining the purpose of each command added.

Finally, it is also worth noting that there is a Rustup installation in the Tiflash Dockerfile. While this is not necessarily a problem, it may be worth ensuring that this is necessary for the CI/CD pipeline.

Copy link

ti-chi-bot bot commented Oct 23, 2024

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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 merged commit 69874d6 into PingCAP-QE:main Oct 23, 2024
12 checks passed
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