-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Use the mirror in China for Azure China environment #27005
Conversation
Thank you for your contribution! I think this may need some extra eyes, as I don't know if there's objections against referring to a third-party mirror ping @kencochrane @tianon @tibor |
@thaJeztah Thank you for the comment, we had some previous communication and agreement with Docker about this mirror. |
Thanks for the PR! Confirming that this was discussed previously and agree that there were no major objections raised. This should still go through the regular review process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the idea sounds sane (and thanks to GPG, should be generally as safe as install.sh
ever is), just some minor style comments. 👍
if [ ! -z "$environment" ] && [ $environment = "AzureChinaCloud" ]; then | ||
apt_url="https://mirror.azure.cn/docker-engine/apt" | ||
yum_url="https://mirror.azure.cn/docker-engine/yum" | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd make sense to move this whole block up closer to the initial setting of these variables (so we keep our constants all in one place, especially including the new args to the script this introduces).
esac | ||
done | ||
|
||
if [ ! -z "$environment" ] && [ $environment = "AzureChinaCloud" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$environment
should be quoted here, and it might actually make sense to convert this to a case
for simplicity, ie:
case "$environment" in
AzureChinaCloud)
....
;;
esac
while getopts "e:" opt; do | ||
case $opt in | ||
e) | ||
environment=$OPTARG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable ought to be initialized before this loop starts to ensure we don't get side effects from the enclosing shell, ie environment=
on a line by itself right before the while
starts.
Thank you for reviewing. Comments addressed. |
environment='' | ||
while getopts "e:" opt; do | ||
case $opt in | ||
e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the option should have a more descriptive name (for example, -mirror
?) @tianon wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thaJeztah can we use --mirror | -m ?
Yeah, "--mirror" sounds good to me! 👍
|
Please let me know if you have more comments, thanks. |
@lizzha can you squash Your commits so that there's only a single commit in this PR? |
Signed-off-by: Liz Zhang <[email protected]> Use the mirror in China for Azure China environment Signed-off-by: Liz Zhang <[email protected]> Update option name to --mirror Signed-off-by: Liz Zhang <[email protected]> Update indent and change variable name Signed-off-by: Liz Zhang <[email protected]>
Done :) |
ping @tianon for final review / merge |
LGTM |
Thank you! When will https://get.docker.com/ get refreshed with this change? |
@lizzha usually that script is updated with each release, and this just missed the 1.12.2 release; in exceptional cases we can update the script manually, but I'll have to ask if that applies here. |
I have 2 questions: May I ask how to use this
|
@twang2218 once https://get.docker.com gets updated, you could use "curl -sSL https://get.docker.com/ | sh -s -- --mirror AzureChinaCloud" to install docker in China |
I recall there's a co-op between Docker Inc. and Alibaba Cloud to bring Docker Hub on Alibaba Cloud for China Mainland, how's that going? |
@ushuz this repo is for the open source "docker" project, I don't have additional information about that |
Created a PR #28858 to add Alibaba Cloud Docker apt/yum mirror support to 'hack/install.sh' |
We've set up a mirror of docker installation packages in Azure China, which syncs with docker repo every day, to mitigate the pain due to connection issues from China to docker official repo.
A new parameter is added in this change. If '-e AzureChinaCloud' is specified, the script will use the packages from the mirror in Azure China.