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(tidbx): add dockerfile #560

Merged
merged 1 commit into from
Feb 24, 2025
Merged

feat(tidbx): add dockerfile #560

merged 1 commit into from
Feb 24, 2025

Conversation

wuhuizuo
Copy link
Contributor

No description provided.

@ti-chi-bot ti-chi-bot bot requested a review from purelind February 24, 2025 08:13
Copy link

ti-chi-bot bot commented Feb 24, 2025

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

Key Changes:

  1. Added a new Dockerfile for tidbx in the directory dockerfiles/cd/builders/tidbx/centos7.
  2. Added a new configuration file skaffold-centos7.yaml for building tidbx image using Skaffold.

Potential Problems:

  1. The Dockerfile and Skaffold configuration seem to be incomplete. The final image stage is commented out, and the copy command is missing. It needs to be completed for the build to work correctly.
  2. The profiles section in skaffold-centos7.yaml specifies go-1.23, but in the Dockerfile, the GOLANG_VERSION is set to 1.23.6. There might be inconsistency in the Golang version used.

Fixing Suggestions:

  1. Complete the Dockerfile by uncommenting the final image stage, adding the necessary copy command, and ensuring the image is built correctly.
  2. Ensure consistency in the Golang version specified in the Dockerfile and Skaffold configuration to avoid any compatibility issues.

Overall, the changes look good, but the completeness and consistency of the Dockerfile and Skaffold configuration need to be verified before merging the pull request.

@ti-chi-bot ti-chi-bot bot added the size/L label Feb 24, 2025
@wuhuizuo wuhuizuo force-pushed the fix/add-lite-builder branch from 2dd4a15 to 76a97ff Compare February 24, 2025 08:14
Copy link

ti-chi-bot bot commented Feb 24, 2025

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

Key Changes:

  1. Added a Dockerfile for tidbx in the path dockerfiles/cd/builders/tidbx/centos7/Dockerfile.
  2. Updated the workflows pull-cd-builder-images-centos7.yaml and release-cd-builder-images-centos7.yaml to include tidbx in the list of builders.
  3. Added a Skaffold configuration for tidbx in the path dockerfiles/cd/builders/tidbx/skaffold-centos7.yaml.

Potential Problems:

  1. Duplicate Entries in Workflow Files:

    • In both workflow files (pull-cd-builder-images-centos7.yaml and release-cd-builder-images-centos7.yaml), the tidbx builder with various profiles (go-1.22, go-1.21, go-1.20, go-1.19) is listed multiple times. This duplication might lead to confusion or unintended behavior.
  2. Hardcoded Golang Version in Dockerfile:

    • The Dockerfile for tidbx specifies a hardcoded Golang version (ARG GOLANG_VERSION=1.23.6). Hardcoding the version may cause issues with future updates or when building in different environments that require a different Golang version.
  3. Skaffold Profile Naming:

    • The Skaffold profile go-1.23 is mentioned in the Skaffold configuration but the corresponding Golang version used in the Dockerfile is 1.23.6. The naming consistency between the Skaffold profile and the Golang version should be maintained.

Suggestions for Fixing:

  1. Workflow File Updates:

    • Remove the duplicate entries for tidbx in the workflow files and ensure that each builder is listed only once with its respective profiles.
  2. Dynamic Golang Version in Dockerfile:

    • Consider using a dynamic way to set the Golang version in the Dockerfile, such as passing it as a build argument or using a variable that can be easily updated when needed.
  3. Skaffold Profile Naming Consistency:

    • Ensure that the naming convention for the Skaffold profiles matches the Golang version being used in the Dockerfile. Update the Skaffold profile names accordingly to maintain consistency.

Overall, the changes look good, but addressing the potential problems mentioned above will help improve the clarity, maintainability, and flexibility of the CI/CD process.

@ti-chi-bot ti-chi-bot bot added size/M and removed size/L labels Feb 24, 2025
@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Feb 24, 2025

[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 Feb 24, 2025
@ti-chi-bot ti-chi-bot bot merged commit 06c259f into main Feb 24, 2025
5 checks passed
@ti-chi-bot ti-chi-bot bot deleted the fix/add-lite-builder branch February 24, 2025 08:23
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