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

ci: split skaffold module for tiflash builder image #166

Merged
merged 3 commits into from
Dec 18, 2023

Conversation

wuhuizuo
Copy link
Contributor

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

keep it same style with `git describe --tags --dirty`

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Copy link

ti-chi-bot bot commented Dec 17, 2023

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

Review for PR: ci: split skaffold module for tiflash builder image

Summary

This pull request adds a new module for Skaffold to build the tiflash image. It also adds this new module to the existing matrix of modules. Additionally, it adds a fetch-depth and fetch-tags parameter to some of the workflows.

Potential problems

There are no obvious potential problems with this pull request.

Suggestions

None.

Overall, this is a simple and straightforward change that appears to be well-implemented. I recommend merging it.

@ti-chi-bot ti-chi-bot bot requested review from jayl1e and purelind December 17, 2023 07:57
@ti-chi-bot ti-chi-bot bot added the size/M label Dec 17, 2023
Signed-off-by: wuhuizuo <wuhuizuo@126.com>

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
Copy link

ti-chi-bot bot commented Dec 17, 2023

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

Review of "ci: split skaffold module for tiflash builder image"

Summary

This pull request adds a new module builder-others-slow to the existing Skaffold configuration. This new module is responsible for building the Tiflash builder image. The builder-others-slow module is added to the pull-cd-builder-images.yaml and release-cd-builder-images.yaml workflows. Also, a new workflow pull-verify-packages-config.yaml is added to test the packages configuration.

Potential Problems

  1. The new pull-verify-packages-config.yaml workflow tests all packages in parallel. This can cause a high load on the CI system and can result in a slower build.
  2. The fetch-depth and fetch-tags options are added to the workflows release-cd-builder-images.yaml, release-cd-util-images.yaml, release-ci-runtime-images.yaml, and release-prod-runtime-images.yaml. This can cause unnecessary cloning of the repository, which can slow down the build.
  3. The tagPolicy configuration is removed from the Skaffold configuration for all modules except the builder-go module. This can cause issues with tagging the built images.

Fixing Suggestions

  1. To avoid high load on the CI system, it's recommended to split the test jobs into multiple workflows or reduce the number of parallel tests.
  2. Instead of always fetching all tags, it's suggested to use fetch-depth: 1 to fetch only the latest commit and its parents. This will reduce the time needed for cloning the repository.
  3. To avoid issues with tagging the built images, it's recommended to keep the tagPolicy configuration for all modules.

Overall

The changes in this pull request seem reasonable, but there are some potential problems that need to be addressed. The suggested fixes should be considered to avoid any issues during the build process.

@ti-chi-bot ti-chi-bot bot added size/L and removed size/M labels Dec 17, 2023
@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Dec 17, 2023

[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 Dec 17, 2023
@wuhuizuo wuhuizuo merged commit 9f4fddb into main Dec 18, 2023
19 of 21 checks passed
@wuhuizuo wuhuizuo deleted the fix/refactor-slow-tiflash-builder-flow branch December 18, 2023 05:34
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