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

refactor(dockerfiles,dockerfiles-multi-stages): simple builder image for pd #111

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

wuhuizuo
Copy link
Contributor

@wuhuizuo wuhuizuo commented Nov 8, 2023

  • make the base image open source, also make it able to be updated by renovate bot.
  • simple the builder image to building pd.

@ti-chi-bot ti-chi-bot bot requested review from jayl1e and purelind November 8, 2023 12:43
@ti-chi-bot ti-chi-bot bot added the size/XL label Nov 8, 2023
Copy link

ti-chi-bot bot commented Nov 8, 2023

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

Pull Request Review

Title:

refactor(dockerfiles,dockerfiles-multi-stages): simple builder image for pd

Description:

- make the base image open source, also make it able to be updated by renovate bot.
- simple the builder image to building `pd`.

Summary of Key Changes:

  1. Removed unnecessary content from README.md and added a new line to point to the new README.md in the dockerfiles/cd directory.
  2. Deleted dockerfiles-multi-stages/pd/Dockerfile.
  3. Updated dockerfiles/bases/skaffold.yaml to change the template from v1.7.0-tmp to v1.7.0.
  4. Added a new dockerfiles/cd/README.md file containing the instructions to build images from source.
  5. Added a new dockerfiles/cd/tikv/pd/Dockerfile file for building the pd image.

Potential Problems:

  1. The new dockerfiles/cd/tikv/pd/Dockerfile is using centos:7.9.2009 as the builder image. It might cause compatibility issues if the previous builder image was using a different version of CentOS.
  2. The new dockerfiles/cd/tikv/pd/Dockerfile is using GOLANG_VERSION=1.21.4. If the previous Golang version was different, it might lead to build failures or compatibility issues.
  3. The new dockerfiles/cd/README.md file contains instructions for building images from source, but it's not clear if these instructions are complete or accurate.

Fixing Suggestions:

  1. Ensure that the new builder image (centos:7.9.2009) is compatible with the previous builder image.
  2. Verify that the Golang version (1.21.4) used in the new dockerfiles/cd/tikv/pd/Dockerfile is compatible with the previous Golang version.
  3. Review the new dockerfiles/cd/README.md file and update the instructions as needed to ensure they are accurate and complete.

Copy link

ti-chi-bot bot commented Nov 8, 2023

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

Summary of Changes

  1. The base image is made open source and able to be updated by the renovate bot.
  2. The builder image for pd is simplified.
  3. The CI and release workflows are updated to detect changes in Dockerfiles using dorny/paths-filter.

Potential Problems

  1. The new change detection in CI and release workflows may not work as expected. Test the workflows to ensure the change detection works correctly.

Suggestions

  1. Test the CI and release workflows to ensure the change detection works correctly.
  2. Verify that the new builder image for pd works as expected.
  3. Make sure that the updated base image works correctly with the other components of the project.

Review

- The base image is made open source, and it can now be updated by the renovate bot.
- The builder image for `pd` has been simplified.
- CI and release workflows have been updated to detect changes in Dockerfiles using `dorny/paths-filter`.

**Potential Problems:**
- The new change detection in CI and release workflows may not work as expected.

**Suggestions:**
1. Test the CI and release workflows to ensure the change detection works correctly.
2. Verify that the new builder image for `pd` works as expected.
3. Make sure that the updated base image works correctly with the other components of the project.

@wuhuizuo wuhuizuo force-pushed the opt/simple-cd-base-image-for-pd branch from 173a163 to ee1559b Compare November 9, 2023 02:32
Copy link

ti-chi-bot bot commented Nov 9, 2023

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

Summary

This pull request focuses on refactoring the Dockerfiles and multi-stage builds for the PD (Placement Driver) component. Key changes include:

  1. Making the base image open source and allowing it to be updated by the renovate bot.
  2. Simplifying the builder image for building the PD component.

Changes

The following changes were made in the .github/workflows/ci.yaml and .github/workflows/release.yaml files:

  • A new job dockerfiles-changes was added to detect changes in Dockerfiles.
  • The build-product-bases, build-ci-bases, and build-ci-utils jobs now have a needs dependency on the dockerfiles-changes job.
  • The build matrix for these jobs is now generated dynamically based on the output from the dockerfiles-changes job.

In the README.md file:

  • The content was replaced with a brief description and a link to the new dockerfiles/cd/README.md file.

In the dockerfiles/bases/skaffold.yaml file:

  • The custom template tag was updated to v1.7.0.

The dockerfiles-multi-stages/pd/Dockerfile file was deleted and a new dockerfiles/cd/tikv/pd/Dockerfile file was created with similar content.

Potential problems and suggestions

  1. In the build-product-bases, build-ci-bases, and build-ci-utils jobs, the if conditions were removed. This means that these jobs will always run, even if there are no changes in the respective Dockerfiles. If it is desired to only run these jobs when there are changes in the Dockerfiles, the if conditions should be added back with the appropriate output from the dockerfiles-changes job.

  2. The dockerfiles-changes job only detects changes in Dockerfiles for the pull request. If the purpose of this job is also to detect changes in the base branch, additional logic should be added to compare the base branch and the head branch.

  3. The new dockerfiles/cd/tikv/pd/Dockerfile file uses the centos:7.9.2009 image as the builder. It is recommended to use an official golang image as the builder, which already includes the golang toolchain and simplifies the build process.

  4. The GOLANG_VERSION argument is hardcoded in the new dockerfiles/cd/tikv/pd/Dockerfile. It is recommended to use build arguments or environment variables to allow easy updates to the golang version.

Overall, the pull request looks good, but the potential problems mentioned above should be considered and addressed if necessary.

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

ti-chi-bot bot commented Nov 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lijie0123

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 9, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-11-09 03:50:09.431081861 +0000 UTC m=+3702607.018192007: ☑️ agreed by lijie0123.

@ti-chi-bot ti-chi-bot bot added the approved label Nov 9, 2023
@ti-chi-bot ti-chi-bot bot merged commit b6b8db9 into main Nov 9, 2023
3 checks passed
@ti-chi-bot ti-chi-bot bot deleted the opt/simple-cd-base-image-for-pd branch November 9, 2023 03:51
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