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.

Sign up for GitHub

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

Jump to bottom

combine prepare_env scripts into one #38

Merged
merged 8 commits into from
Dec 25, 2020
Merged

combine prepare_env scripts into one #38

merged 8 commits into from
Dec 25, 2020

Conversation

acezen
Copy link
Collaborator

@acezen acezen commented Dec 23, 2020

What do these changes do?

combine the prepare_env.sh and prepare_env_wsl.sh into one

Related issue number

Fixes #33

@acezen acezen changed the title combine prepare_env script into one combine prepare_env scripts into one Dec 23, 2020
@acezen
Copy link
Collaborator Author

acezen commented Dec 23, 2020

cc/ @yecol @sighingnow please review and give some comments.

sighingnow
sighingnow approved these changes Dec 23, 2020
View reviewed changes
Copy link
Collaborator

@sighingnow sighingnow left a comment

Choose a reason for hiding this comment

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

LGTM, just a little concern about WSL_DISTRO_NAME or IS_WSL? Are they guaranteed to exist under WSL2 environment?

@sighingnow sighingnow requested a review from yecol December 23, 2020 11:28
siyuan0322
siyuan0322 reviewed Dec 23, 2020
View reviewed changes
scripts/prepare_env.sh Outdated Show resolved Hide resolved
@acezen
Copy link
Collaborator Author

acezen commented Dec 23, 2020

LGTM, just a little concern about WSL_DISTRO_NAME or IS_WSL? Are they guaranteed to exist under WSL2 environment?

there is a issue in WSL that suggest to use the env to check is in WSL or not. microsoft/WSL#4555 (comment)

sighingnow
sighingnow approved these changes Dec 23, 2020
View reviewed changes
scripts/prepare_env.sh Outdated Show resolved Hide resolved
@sighingnow sighingnow self-requested a review December 24, 2020 02:31
sighingnow
sighingnow requested changes Dec 24, 2020
View reviewed changes
Copy link
Collaborator

@sighingnow sighingnow left a comment

Choose a reason for hiding this comment

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

Please update README.md which refers prepare_env_wsl.sh (and maybe tutorial, references, etc.)

image

yecol
yecol requested changes Dec 24, 2020
View reviewed changes
Copy link
Collaborator

@yecol yecol left a comment

Choose a reason for hiding this comment

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

I suggest the script could be more verbose.

  • tell user what is doing.
  • give a summary at the end of the entire process.

e.g.,

minikube VERSION installed.
# or 
kind VERSION installed.
dockerd started.
...

scripts/prepare_env.sh Show resolved Hide resolved
@acezen
Copy link
Collaborator Author

acezen commented Dec 24, 2020

I suggest the script could be more verbose.

  • tell user what is doing.
  • give a summary at the end of the entire process.

e.g.,

minikube VERSION installed.
# or 
kind VERSION installed.
dockerd started.
...

already add some verbose log and a summary to script, please review again

@yecol yecol merged commit 595d1c7 into alibaba:main Dec 25, 2020
@acezen acezen deleted the zwb/combine_script branch September 3, 2021 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siyuan0322 siyuan0322 siyuan0322 left review comments

@sighingnow sighingnow sighingnow approved these changes

@yecol yecol yecol approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

combine prepare_env.sh and prepare_env_wsl.sh into one script
4 participants
@acezen @yecol @sighingnow @siyuan0322