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(apps/prod/tekton/configs/tasks): update worked rustup mirror #1266

Merged
merged 2 commits into from
Sep 25, 2024

Conversation

wuhuizuo
Copy link
Collaborator

builds failed in fetch the date nightly toolchain

Signed-off-by: wuhuizuo [email protected]

builds failed in fetch the date nightly toolchain

Signed-off-by: wuhuizuo <[email protected]>
@ti-chi-bot ti-chi-bot bot requested a review from purelind September 25, 2024 08:11
@ti-chi-bot ti-chi-bot bot added area/apps env/prod will deploy on the main product cluster labels Sep 25, 2024
Copy link
Contributor

ti-chi-bot bot commented Sep 25, 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 disabling the rustup mirror to fix build failures.

The key changes in the diff are that the lines setting the rustup mirror have been commented out in three different YAML files. This change will disable the use of the mirror for rustup, which might result in slower downloads of Rust toolchain files.

There are no new problems introduced by this pull request, but there is a potential problem with the change. If the original build failures were caused by slow downloads from the default Rustup server, disabling the mirror might make the problem worse. It is also possible that the mirror was not the cause of the build failures, and disabling it might not help.

To fix this, it might be better to investigate the root cause of the build failures and address that instead of simply disabling the rustup mirror. If disabling the mirror is the best solution, it would be good to document the reason for this change and monitor the build times to ensure that there are no negative impacts.

In summary, the pull request disables the rustup mirror to fix build failures, but it might not be the best solution. Investigating the root cause of the build failures is recommended, and if disabling the mirror is the best solution, it should be documented and monitored.

@ti-chi-bot ti-chi-bot bot added the size/S label Sep 25, 2024
Copy link
Contributor

ti-chi-bot bot commented Sep 25, 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, it seems that the PR intends to fix a build failure issue by disabling a rustup mirror. The diff shows that the PR modifies the pingcap-build-binaries-darwin.yaml, pingcap-build-binaries.yaml, and create-pr-to-bump-tikv-version.yaml files, replacing the https://mirrors.tuna.tsinghua.edu.cn with https://rsproxy.cn for RUSTUP_DIST_SERVER and RUSTUP_UPDATE_ROOT environment variables.

Potential problems:

  • It is not clear from the PR description what caused the build failures. It would be helpful to provide more context or error messages to understand why the rustup mirror is being disabled.
  • The PR modifies multiple files, which might have unintended consequences and make it difficult to isolate any issues that arise from the changes.
  • It is not clear whether the https://rsproxy.cn mirror is reliable and secure.

Fixing suggestions:

  • Provide more context or error messages to help reviewers understand the reason for the changes.
  • Split the PR into smaller ones that modify one file at a time. This will reduce the risk of introducing unintended consequences and make it easier to isolate issues.
  • Check whether https://rsproxy.cn is a reliable and secure mirror before making the changes, and consider other mirrors if there are concerns.

@wuhuizuo
Copy link
Collaborator Author

/approve

Copy link
Contributor

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

@ti-chi-bot ti-chi-bot bot added the approved label Sep 25, 2024
@ti-chi-bot ti-chi-bot bot merged commit 52bc780 into main Sep 25, 2024
4 checks passed
@ti-chi-bot ti-chi-bot bot deleted the fix/disable-rustup-mirrors branch September 25, 2024 08:39
@wuhuizuo wuhuizuo changed the title fix(apps/prod/tekton/configs/tasks): disable rustup mirror fix(apps/prod/tekton/configs/tasks): update worked rustup mirror Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/apps env/prod will deploy on the main product cluster size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant