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): archive old centos builder images #444

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

wuhuizuo
Copy link
Contributor

Signed-off-by: wuhuizuo wuhuizuo@126.com

@ti-chi-bot ti-chi-bot bot requested a review from purelind September 30, 2024 12:57
@ti-chi-bot ti-chi-bot bot added the size/XXL label Sep 30, 2024
Copy link

ti-chi-bot bot commented Sep 30, 2024

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

The pull request makes several changes to the continuous integration/continuous deployment (CI/CD) workflow files and Dockerfiles related to building images for different services. Here's a summary of the changes:

  1. Two new GitHub workflow files have been added - pull-cd-builder-images-centos7.yaml and release-cd-builder-images-centos7.yaml. These files define the workflow for building images when a pull request is made and when a release is made, respectively.

  2. Changes have been made to the workflows defined in pull-cd-builder-images.yaml and release-cd-builder-images.yaml - mainly the paths that trigger the workflows and the build steps.

  3. The cache layer for Docker images has been removed in the workflows.

  4. The Dockerfiles for building images for different services have been updated and new ones have been added. This includes updates to the base images used, the build arguments, and the steps to build the images.

Potential problems:

  1. The removal of the cache layer for Docker images can slow down the build process as Docker has to pull all the layers every time an image is built.

  2. The modifications to the paths that trigger the workflows in pull-cd-builder-images.yaml and release-cd-builder-images.yaml could potentially cause the workflows to not trigger when they should.

Fixing suggestions:

  1. If the build process is too slow without the cache layer, consider adding it back.

  2. Test the workflows to ensure they are triggered correctly based on the modified paths.

  3. Check the Dockerfiles to ensure the base images used are up-to-date and the build arguments are correct.

  4. Test the images built from the Dockerfiles to ensure they work as expected.

Copy link

ti-chi-bot bot commented Sep 30, 2024

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

The pull request contains changes related to:

  1. Dockerfile updates: It adds new Dockerfiles for building different components like ng-monitoring, pd, tidb-dashboard, tikv, tiflow, and tidb based on CentOS 7.

  2. GitHub Actions workflows: New workflows are added for building and publishing Continuous Delivery (CD) builder images using Skaffold. These workflows are triggered on pull requests and when there are changes to the main branch. The workflows use GitHub's own package registry (ghcr.io) to store the built images.

  3. Skaffold configuration: New Skaffold configuration files are added for building the images.

Here are the potential issues and suggestions:

  1. Hardcoded Go versions: The Go versions are hardcoded in the Skaffold configuration. It might be a good idea to have these versions defined in a separate configuration file for easier management.

  2. Use of CentOS 7: CentOS 7 has reached End of Life (EOL) on June 30, 2024. Considering moving to a supported OS for security and updates.

  3. Potential concurrency issues: The concurrency level is set to 0 for the local builds in the Skaffold configuration. This means Skaffold will try to build all artifacts in parallel, which might cause issues if the system running the build does not have enough resources. If you notice builds failing due to out of resources errors, consider setting the concurrency level to a fixed number.

  4. Test coverage: Make sure that the new Dockerfiles and workflows are adequately covered by tests.

  5. Docker image size: Consider using multi-stage builds or removing unnecessary files after building the binaries to reduce the size of the final Docker images.

  6. Security: Make sure that the Docker images are scanned for vulnerabilities as part of the CI/CD pipeline. This can be done using tools like Trivy or Anchore.

  7. Error handling in scripts: The scripts in the GitHub Actions workflows should handle errors properly. Right now, if a command fails, the script will continue running. Consider adding set -e at the beginning of your scripts to make them exit on the first error.

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@wuhuizuo wuhuizuo force-pushed the feature/archive-old-builder-in-folders branch from 65cbfce to 8462900 Compare September 30, 2024 15:40
Copy link

ti-chi-bot bot commented Sep 30, 2024

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

The pull request is titled "feat(dockerfiles/cd/builders): archive old centos builder images". It looks like the purpose of this PR is to archive the old CentOS builder images and update the GitHub workflows related to these images.

Key Changes:

  1. Two new GitHub workflow files are created: pull-cd-builder-images-centos7.yaml and release-cd-builder-images-centos7.yaml. These files contain the steps to build and publish CD builder images on CentOS 7.
  2. Two existing GitHub workflow files: pull-cd-builder-images.yaml and release-cd-builder-images.yaml are updated. These workflows now target Dockerfiles under direct child directories of dockerfiles/cd/builders/ instead of any nested directories.
  3. The caching mechanism in the build process of the images is removed in both pull-cd-builder-images.yaml and release-cd-builder-images.yaml.
  4. Several new Dockerfiles are created under centos7 directories for different components, including ng-monitoring, pd, skaffold-centos7.yaml, tidb-dashboard, tidb, tiflash, and tikv.
  5. A new Skaffold configuration file skaffold-centos7.yaml is created to map with the new Dockerfiles.
  6. The Skaffold configuration file skaffold.yaml is updated to match the new Dockerfile paths and remove unnecessary build arguments.

Potential Problems:

  1. No test results or logs are provided in the PR description. This makes it hard to verify if the changes will work as expected.
  2. The CentOS 7 version used in the Dockerfiles has reached EOL (End of Life). If there are any security vulnerabilities found in this version, they won't be patched by the CentOS community. This could pose a security risk.
  3. The removal of caching in the build process may increase the build time.

Fixing Suggestions:

  1. Provide logs or other evidence showing that the changes have been tested and work as expected.
  2. Consider updating to a newer CentOS version that still has community support.
  3. If possible, consider adding a caching mechanism to the build process to potentially speed up the build time.

Copy link

ti-chi-bot bot commented Sep 30, 2024

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

PR Title: "Archive old CentOS builder images"

The PR introduces a new workflow for archiving old CentOS builder images. It adds new GitHub Action workflows for building and publishing images. There are three new workflows for Pull Request, Release, and main branch pushes.

Here are some identified changes:

  1. New GitHub action workflows are added for building and publishing CD builder images.
  2. The new workflows are configured to run on pull requests and pushes to the main branch, and are set to build on changes to Dockerfiles in certain paths.
  3. The new workflows use two jobs to build images with Skaffold. Each job checks out the source code, sets up QEMU and Docker Buildx, logs in to the GitHub Container Registry, sets up Skaffold, and builds the images.
  4. The old workflows and Dockerfiles are updated to match the new setup.
  5. A new Dockerfile is added for each of the following: Ng-Monitoring, PD, Tidb Dashboard, Tidb, Tiflash, and Tikv.

Potential problems:

  1. The workflows are set to run on pull requests and pushes to the main branch. It might be better to limit the running of workflows to specific events or branches to prevent unnecessary runs.
  2. There is a mixture of Docker and Skaffold being used. It would be better to stick with one for consistency and to reduce complexity.
  3. It's not clear if the Dockerfiles are pulling from a secure and trusted source. It's important to ensure that images are not compromised.

Fixing suggestions:

  1. Limit the running of workflows to specific events or branches to prevent unnecessary runs.
  2. Stick with either Docker or Skaffold for building images to reduce complexity.
  3. Ensure that Dockerfiles are pulling from secure and trusted sources.
  4. It would be advisable to have tests in place to ensure the built images work as expected before they are published.

@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Sep 30, 2024

[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 Sep 30, 2024
@ti-chi-bot ti-chi-bot bot merged commit 4478861 into main Sep 30, 2024
14 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feature/archive-old-builder-in-folders branch September 30, 2024 17:28
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