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(pacakges): update the downloading url for etcd binary #252

Merged
merged 2 commits into from
Feb 22, 2024

Conversation

wuhuizuo
Copy link
Contributor

It supports darwin arm64 buiding from v3.5.5 by etcd-io office source.

Ref: etcd-io/etcd#14001
Signed-off-by: wuhuizuo [email protected]

@ti-chi-bot ti-chi-bot bot requested review from jayl1e and purelind February 22, 2024 06:01
Copy link

ti-chi-bot bot commented Feb 22, 2024

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

The pull request updates the downloading URL for the etcd binary in the packages.yaml.tmpl file. The updated URL is for version 3.4.21, and it is hosted on a PingCAP file server. The change is made to support Darwin arm64 building from v3.5.5 by etcd-io office source.

Potential Problems:

It is unclear why the URL for version 3.4.21 is being updated instead of the URL for version 3.5.5, which is the version that supports Darwin arm64 building. Additionally, the comment in the code suggests that the URL for version 3.4.21 should be replaced with the URL for version 3.5.x when upgrading. However, the code does not include the URL for version 3.5.x.

Fixing Suggestions:

  • Clarify in the pull request description why the URL for version 3.4.21 is being updated instead of the URL for version 3.5.5.
  • Add the URL for version 3.5.5 to the code, and update the comment to reflect this change.
  • Consider renaming the pull request title to reflect that the change is for version 3.4.21, not version 3.5.5.

@ti-chi-bot ti-chi-bot bot added the size/XS label Feb 22, 2024
It supports darwin arm64 buiding from v3.5.5 by etcd-io office source.

Ref: etcd-io/etcd#14001
Signed-off-by: wuhuizuo <[email protected]>
@wuhuizuo wuhuizuo force-pushed the fix/etcdctl-darwin-arm64-download-url branch from 327cf82 to 4d66fc9 Compare February 22, 2024 06:08
Copy link

ti-chi-bot bot commented Feb 22, 2024

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

Review for PR: fix(pacakges): update the downloading url for etcd binary

Summary

This pull request updates the downloading URL for the etcd binary in the packages.yaml.tmpl file. It adds support for darwin arm64 building from v3.5.5 by etcd-io office source.

Potential problems

  • There are two url keys defined for src in the etcdctl component. The second key will overwrite the first key, and the etcd binary will be downloaded from the last defined URL. So, the first URL is redundant and should be removed.
  • The url for darwin arm64 is commented out, and the URL for darwin amd64 is still used. This may cause issues for those who need to download the etcd binary for darwin arm64. So, the comment should be removed and the URL for darwin arm64 should be used instead of the URL for darwin amd64.

Fixing suggestions

  • Remove the first url key for the etcdctl component.
  • Uncomment and use the URL for darwin arm64:
# ${FILE_SERVER_URL}/download/pingcap/etcd-${ETCDCTL_VERSION}-${os}-${arch}.tar.gz
# Notice: it only support darwin arm64 platform from [v3.5.5](https://github.com/etcd-io/etcd/releases/tag/v3.5.5) in office artifacts.
# When we upgrade to 3.5.x, we should replace it with the following url:
# "https://github.com/etcd-io/etcd/releases/download/v3.4.21/etcd-v3.4.21-{{ .Release.os }}-{{ .Release.arch }}.zip"
# Also we need support extract from zip archive.
- url: "http://fileserver.pingcap.net/download/pingcap/etcd-v3.4.21-{{ .Release.os }}-{{ .Release.arch }}.tar.gz"
+ url: "https://github.com/etcd-io/etcd/releases/download/v3.5.5/etcd-v3.5.5-{{ .Release.os }}-{{ .Release.arch }}.tar.gz"

@ti-chi-bot ti-chi-bot bot added size/S and removed size/XS labels Feb 22, 2024
Copy link

ti-chi-bot bot commented Feb 22, 2024

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

Review

This pull request updates the downloading URL for etcd binary in the packages.yaml.tmpl file. It adds support for building darwin arm64 from version 3.5.5 of etcd-io office source.

Potential Problems

There seem to be no potential issues with this pull request. However, it would be helpful to add some testing instructions to ensure that the changes work as expected.

Fix Suggestions

Since the changes are minimal, the pull request seems fine as it is. However, the author could add testing instructions and also update the pull request title to be more descriptive. For example, something like "feat(packages): Add support for darwin arm64 building from etcd-io office source" would be more informative.

@ti-chi-bot ti-chi-bot bot added size/XS and removed size/S labels Feb 22, 2024
@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Feb 22, 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 Feb 22, 2024
@ti-chi-bot ti-chi-bot bot merged commit fbe8459 into main Feb 22, 2024
2 checks passed
@ti-chi-bot ti-chi-bot bot deleted the fix/etcdctl-darwin-arm64-download-url branch February 22, 2024 06:22
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