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(dockerfiles/cd/builders/tiflash): update image docker file for new tiflash builder #484

Merged
merged 5 commits into from
Nov 12, 2024

Conversation

wuhuizuo
Copy link
Contributor

@wuhuizuo wuhuizuo commented Nov 7, 2024

Signed-off-by: wuhuizuo [email protected]

@ti-chi-bot ti-chi-bot bot requested a review from purelind November 7, 2024 03:53
Copy link

ti-chi-bot bot commented Nov 7, 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 following are the key changes:

  • The Dockerfile for tiflash builder has been updated.
  • The builder stage has been removed.
  • The extract-conda-openssl stage has been removed.
  • The llvm toolset installation has been removed.
  • The rust toolchain installation has been removed.
  • The timezone has been set to Asia/Shanghai.
  • The PATH, LIBRARY_PATH, LD_LIBRARY_PATH, CPLUS_INCLUDE_PATH, and OPENSSL_ROOT_DIR environment variables have been set.
  • The TIFLASH_REPO and TIFLASH_REF arguments have been added.
  • The building stage has been updated to use the TIFLASH_REPO and TIFLASH_REF arguments.
  • The ADD command has been updated to copy the files to /ws instead of /tiflash.
  • The output directory has been changed to /ws/output.

Potential problems:

  • There is no clear description of the changes made in this pull request, which can make it difficult to understand the purpose of the changes.
  • The previous stages such as extract-conda-openssl and builder have been removed, which may have an impact on the functionality of the Dockerfile.
  • The timezone has been set to Asia/Shanghai, which may not be desirable for all users.
  • The removal of the rust toolchain may affect the compilation of some packages.

Fixing suggestions:

  • Add a clear description of the changes made in this pull request to make it easier for reviewers to understand the purpose of the changes.
  • Provide an explanation for the removal of the extract-conda-openssl and builder stages, and ensure that their functionality has been replaced or is no longer needed.
  • Consider making the timezone setting configurable so that users can choose their desired timezone.
  • Provide an explanation for the removal of the rust toolchain, and ensure that it will not affect the compilation of packages that depend on it.

@ti-chi-bot ti-chi-bot bot added the size/L label Nov 7, 2024
Copy link

ti-chi-bot bot commented Nov 7, 2024

@Lloyd-Pottiger: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link

ti-chi-bot bot commented Nov 8, 2024

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Copy link

ti-chi-bot bot commented Nov 11, 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 are:

  • The Dockerfile for the tiflash builder has been updated to use the official repo and tag.
  • The base image has been updated to RockyLinux 8.10.
  • Rust toolchain has been removed.
  • The builder image is now using LLVM 17.0.6.

One potential problem is that the previous version of the Dockerfile had a stage for extracting conda-openssl, which seems to have been removed in this pull request. It would be good to check if this is still needed and if there are any implications for removing it.

Another issue is that the pull request changes the timezone to Asia/Shanghai, which may not be suitable for all use cases.

To fix these issues, I would suggest:

  • Investigating if the extract-conda-openssl stage is still needed and adding it back if necessary.
  • Making the timezone configurable or leaving it as the default.
  • Considering adding comments to the Dockerfile to explain the reasoning behind the changes.

Copy link

ti-chi-bot bot commented Nov 11, 2024

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

This pull request updates the Dockerfile for the TiFlash builder and includes the following changes:

  • Removes the stage "extract-conda-openssl" and installs OpenSSL with a pre-built package.
  • Replaces the installation of LLVM and Clang with a script from the official TiFlash repository.
  • Updates the timezone and environment variables.
  • Replaces the installation of Rust with the use of a pre-installed version.
  • Fixes a typo in the pr description.

It appears that the changes made in this pull request should not cause any problems. However, there are some recommendations to improve the code:

  • Instead of using a pre-built OpenSSL package, it is recommended to build it from source in the same Dockerfile.
  • Instead of using a script from the TiFlash repository to install LLVM and Clang, it is recommended to use the official LLVM and Clang Docker images.
  • It is recommended to use a specific version of Rust instead of a pre-installed version.
  • The commit message should contain more information about the changes made.

Overall, the changes in this pull request seem reasonable, and the reviewer can approve it. However, the recommendations mentioned above can improve the code quality and maintainability.

@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Nov 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Lloyd-Pottiger, 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

Copy link

ti-chi-bot bot commented Nov 12, 2024

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

Based on the PR title and description, it seems that this PR is updating the Dockerfile for TiFlash builder to support a new TiFlash builder.

The key changes made in the diff are:

  • Removed a stage (extract-conda-openssl) from the Dockerfile.
  • Updated the base image for the builder stage.
  • Replaced the TiFlash repository and tag with the corresponding environment variables.
  • Updated the environment variables for Timezone, PATH, LIBRARY_PATH, LD_LIBRARY_PATH, CPLUS_INCLUDE_PATH, and OPENSSL_ROOT_DIR.
  • Removed the installation of LLVM toolsets, OpenSSL, and Rust Toolchain from the Dockerfile.
  • Updated the ADD command to copy the files to /ws/ instead of /tiflash.
  • Updated the build command from /tiflash/release-centos7-llvm/scripts/build-release.sh to CARGO_NET_GIT_FETCH_WITH_CLI=true /ws/release-linux-llvm/scripts/build-release.sh.
  • Updated the output directory from /tiflash/output/tiflash to /ws/output/tiflash.

As for potential problems, it's hard to say without knowing the context and purpose of this Dockerfile and the TiFlash builder. However, some suggestions for fixing or improving the Dockerfile are:

  • Review the removed stage (extract-conda-openssl) to ensure that it is not necessary for the new TiFlash builder.
  • Make sure that the new base image is compatible with the TiFlash builder and all dependencies.
  • Ensure that the updated environment variables are correct and necessary for the TiFlash builder.
  • Consider adding comments to the Dockerfile to explain the purpose of each stage and command.
  • Consider adding a version tag for the new TiFlash builder to make it easier to track and manage.

@wuhuizuo wuhuizuo merged commit 7bd274e into main Nov 12, 2024
24 of 26 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feature/update-tiflash-builder branch November 12, 2024 11:33
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.

3 participants