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): add go-1.23 and tiflash in to build workflows #394

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

wuhuizuo
Copy link
Contributor

@wuhuizuo wuhuizuo commented Sep 2, 2024

Signed-off-by: wuhuizuo [email protected]

@ti-chi-bot ti-chi-bot bot requested a review from purelind September 2, 2024 06:17
Copy link

ti-chi-bot bot commented Sep 2, 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, it seems that the changes add support for two new versions, go-1.23 and tiflash, in the build workflows. The changes are mainly in the pull-cd-builder-images.yaml and release-cd-builder-images.yaml workflows, as well as in the skaffold.yaml file in the dockerfiles/cd/builders directory.

Potential problems that I can identify include:

  • The new version of go (go-1.23) has not been tested yet, and there is no information on whether it is compatible with the existing codebase.
  • It is also unclear why the order of the go versions in the matrix has been changed, which could potentially cause issues with the build process.
  • The changes to the skaffold.yaml file seem to be adding a new builder profile for tiflash, but there is no explanation on what this profile is for or how it fits into the existing build system.

Here are some suggestions for fixing the potential problems:

  • Before adding support for a new version of go, it is important to test it thoroughly to ensure that it is compatible with the existing codebase. This can be done by running the tests on a branch that uses the new version of go.
  • If the order of the go versions in the matrix has been changed for a specific reason, it should be explained in the pull request description to avoid confusion.
  • The changes to the skaffold.yaml file should be explained in more detail, including what the new tiflash builder profile is for and how it should be used.

@ti-chi-bot ti-chi-bot bot added the size/M label Sep 2, 2024
@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Sep 2, 2024

/approve

@ti-chi-bot ti-chi-bot bot added the approved label Sep 2, 2024
@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Sep 2, 2024

/approve

@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Sep 2, 2024

/hold

@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Sep 3, 2024

/approve

Copy link

ti-chi-bot bot commented Sep 3, 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

@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Sep 3, 2024

/unhold

@ti-chi-bot ti-chi-bot bot merged commit 2ed3ea4 into main Sep 3, 2024
19 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feature/add-go1.23-builds branch September 3, 2024 11:59
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