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

fix(tiflash): fix script path #3216

Closed
wants to merge 2 commits into from

Conversation

Lloyd-Pottiger
Copy link
Contributor

In pingcap/tiflash#9587, we modify the directory name.

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>
@ti-chi-bot ti-chi-bot bot requested a review from purelind November 8, 2024 07:15
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 pull request, it seems that the changes are related to fixing the script path in the CI/CD pipeline for the TiFlash project. Specifically, the directory name was modified and the script paths were updated accordingly.

One potential problem could be if there are other scripts or files that reference the old directory name, they may break after this change. It would be good to check if there are any other dependencies that need to be updated as well.

As for fixing suggestions, it would be helpful to add some comments to the code to explain why the changes were made and what dependencies were updated. Additionally, it would be good to perform some testing to ensure that the CI/CD pipeline still runs smoothly after the changes. If any issues arise, they should be addressed promptly.

@ti-chi-bot ti-chi-bot bot requested a review from wuhuizuo November 8, 2024 07:15
Copy link

ti-chi-bot bot commented Nov 8, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign wuhuizuo for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>
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 title and description of the pull request, it seems that the only change made was to modify the path to the script files. Specifically, the directory name was changed from "release-centos7-llvm" to "release-linux-llvm".

There don't seem to be any potential problems with this change as it is only a path modification. However, it is important to ensure that the new path is correct and that the scripts can still be executed without any issues.

As for fixing suggestions, it would be helpful to add more context to the pull request description to explain why the directory name was changed and if any other modifications were made. Additionally, it might be useful to verify that the scripts are still functioning as expected and to update any relevant documentation or instructions to reflect the new path.

wuhuizuo added a commit that referenced this pull request Nov 8, 2024
update the build script path.

Ref #3216

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

purelind commented Nov 8, 2024

duplicate with #3219

ti-chi-bot bot pushed a commit that referenced this pull request Nov 8, 2024
update the build script path.

Ref #3216

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

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@wuhuizuo wuhuizuo closed this Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants