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

chore(tiflash): update build steps for tiflash starts v8.5.0 #488

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

wuhuizuo
Copy link
Contributor

@wuhuizuo wuhuizuo commented Nov 8, 2024

Ref pingcap/tiflash#9587, it changes the build script paths.

Signed-off-by: wuhuizuo [email protected]

Ref pingcap/tiflash#9587, it changes the build script paths.

Signed-off-by: wuhuizuo <[email protected]>
@ti-chi-bot ti-chi-bot bot requested a review from purelind November 8, 2024 07:05
Copy link

ti-chi-bot bot commented Nov 8, 2024

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

From the PR title and description, it seems that the update changes the build steps for tiflash to version 8.5.0. Specifically, it changes the build script path for the range [v8.5.0, ) and adds new steps and artifacts for this version.

One potential problem is that the changes only apply to the specified version range, which means that there might be issues with compatibility or consistency across different versions of tiflash.

To fix this, it might be helpful to provide more context on why the changes were made and whether they have been tested for all supported versions. Additionally, it might be a good idea to add more descriptive comments in the code to explain the rationale behind the changes.

Overall, the changes seem reasonable, but it would be good to ensure that they do not introduce any unexpected issues or regressions.

@ti-chi-bot ti-chi-bot bot added the size/M label Nov 8, 2024
Copy link

ti-chi-bot bot commented Nov 8, 2024

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

This pull request updates the build steps for Tiflash starting from version 8.5.0. It modifies the build script paths and adds new steps to build and package the release artifacts.

One potential problem could be that the changes may not be compatible with older versions of Tiflash. The new steps are only for versions 8.5.0 and above, so older versions may fail to build or package correctly.

To fix this, the pull request could add support for older versions by checking the Tiflash version and applying the appropriate build steps accordingly. This would ensure that all versions of Tiflash can be built and packaged correctly.

Additionally, the pull request could benefit from more detailed documentation on how to use the new build steps and package the release artifacts. This would help other developers who may not be familiar with the updated process.

Overall, the changes look good and will likely improve the build and release process for Tiflash.

@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Nov 8, 2024

/cc @Lloyd-Pottiger

@ti-chi-bot ti-chi-bot bot requested a review from Lloyd-Pottiger November 8, 2024 07:06
packages/packages.yaml.tmpl Outdated Show resolved Hide resolved
Copy link

ti-chi-bot bot commented Nov 8, 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 are related to updating the build steps for Tiflash starting from v8.5.0. The changes include updating the build script paths, adding new build steps with different conditions for different versions, and adding new artifacts for the release.

The changes in the Pull Request seem reasonable and necessary for updating the build steps. However, there are some potential problems that need to be addressed:

  1. The Pull Request does not provide enough context and explanation for the changes made. It would be helpful to include more information about why these changes were necessary, and what impact they will have on the Tiflash build process.
  2. It's not clear if the changes have been tested and verified. It would be helpful to include testing information to ensure that the updated build steps work as expected.

Based on these potential problems, here are some suggestions for fixing them:

  1. Provide more context and explanation for the changes made, including why they were necessary, and what impact they will have on the Tiflash build process.
  2. Include testing information to ensure that the updated build steps work as expected, such as test results, test cases, and test plans.
  3. Consider adding documentation for the updated build steps, including instructions for how to use them, and any potential issues or limitations.

If these suggestions are addressed, the Pull Request should be ready for merging.

release:
- os: linux
script: |
./release-rocky8-llvm/scripts/build-tiflash-release.sh
Copy link
Contributor Author

@wuhuizuo wuhuizuo Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as previous router, just changed the folder name.

Copy link

ti-chi-bot bot commented Nov 8, 2024

@Lloyd-Pottiger: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Nov 8, 2024

/hold

@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Nov 8, 2024

May be we should make the script path be more common. I hold it and convert it to draft state.

@wuhuizuo wuhuizuo marked this pull request as ready for review November 8, 2024 07:26
@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Nov 8, 2024

/unhold

@wuhuizuo wuhuizuo force-pushed the feature/update-tiflash-build-commands-starts-v8.5 branch from 5e9ba3f to c1d98c7 Compare November 8, 2024 07:28
Copy link

ti-chi-bot bot commented Nov 8, 2024

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

Based on the PR title and description, the key change made in this PR is to update the build steps for tiflash starting from version 8.5.0. Specifically, the build script paths have been changed, and the Dockerfile and packages.yaml.tmpl files have been updated accordingly.

One potential problem is that this PR does not provide any information about why the build script paths needed to be changed. If there was a specific reason for this change, it should be documented in the PR description or in a separate issue or documentation.

Another potential problem is that there may be compatibility issues with the updated build steps, especially for users who are upgrading from previous versions of tiflash. It would be helpful to provide guidance on how to handle these compatibility issues, such as any necessary changes to configuration or migration steps.

As for fixing suggestions, if there was a specific reason for the change in build script paths, it should be documented in the PR description or in a separate issue or documentation. If there are any compatibility issues, guidance should be provided on how to handle them. Otherwise, the changes in this PR seem reasonable and should be merged after a thorough review.

@wuhuizuo
Copy link
Contributor Author

wuhuizuo commented Nov 8, 2024

/approve

Copy link

ti-chi-bot bot commented Nov 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Lloyd-Pottiger, 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 Nov 8, 2024
@wuhuizuo wuhuizuo merged commit ae9dd7b into main Nov 8, 2024
25 of 27 checks passed
@wuhuizuo wuhuizuo deleted the feature/update-tiflash-build-commands-starts-v8.5 branch November 8, 2024 08:10
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