-
Notifications
You must be signed in to change notification settings - Fork 102
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
#584: docker + k8s: improved windows and added mac support #593
Conversation
aligned with conventions, removed trailing spaces, fixed bugs, etc.
WSL output is localised, may contain NUL char and gets garbaged, exit code is the only reliable way to check
When I test this on windows it is still not working:
Then nothing happens. The process is stalled forever. BTW: I have choosen Debian as Linux Distribution in case that matters. |
LOL. Indeed this was the problem. Nowhere in the devonfw-ide documentation is stated which linux distribution is required.
However, I now digged in the code again and found this:
So actually the implementation only works for ubuntu. I thought I had been very pickly already in my reviews. Now, I regret that we have already merged this incomplete feature onto
|
else | ||
error="WSL 2 is disabled or no linux distributions found in WSL.\nPlease ensure WSL is enabled and a proper linux distribution is installed in WSL." | ||
fi | ||
doFail "${error}\nTo fix this follow these instructions:\nhttps://docs.microsoft.com/en-us/windows/wsl/install-win10" |
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 would be better to use the short link here. The content will be displayed in the user's language and the link does not beak when MS restructures the docs. https://aka.ms/wsl2-install
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.
@SchettlerKoehler thanks for this feedback.
However, this PR is about the MacOS support so I will not include further windows changes here and better get this merged now.
For windows I will create another PR immediately after merge but there I will entirely remove this and go to Docker Desktop instead what makes that link to MS obsolete. In case we should decide against this PR and Docker Desktop for Windows we should do what you suggested. You can also edit the URL in the browser on github and create a PR from that with 2 clicks if you fear that this change might be forgotten or lost.
arch="$(uname -m)" | ||
if [ "${arch}" = "x86_64" ] | ||
then | ||
downloadUrl="https://desktop.docker.com/mac/stable/amd64/Docker.dmg?utm_source=docker&utm_medium=webreferral&utm_campaign=docs-driven-download-mac-amd64" |
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 you can just us https://desktop.docker.com/mac/stable/amd64/Docker.dmg
The parameters are just google analytics tracking parameters
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.
Excellent feedback. 👍 GET parameters removed.
# Install Docker on WSL | ||
wsl bash ../scripts/setup-docker | ||
fi | ||
downloadUrl="https://desktop.docker.com/mac/stable/arm64/Docker.dmg?utm_source=docker&utm_medium=webreferral&utm_campaign=docs-driven-download-mac-arm64" |
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.
Same here
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.
done
I feel we should support Debian and Ubuntu, nothing more. These are the most often used ones by users, especially windows users, who most often just know these distributions. For sure we can wait for contributions to support more if there are people who want to support more. |
I think we should go with the 2nd one - mentioning in our documentation that we are supporting only these distros rather than not mentioning anything which gives the impression that it should work in any distro. |
Just FYI:- We used the instructions from one of the 1000kit guides |
I will evaluate Docker Desktop for Windows as I feel we are pointlessly reinventing the wheel here. Feedback will follow. |
If DockerDesktop is installed on Windows - its anyways accessible from the WSL. |
I tested the Docker Desktop installation on Windows:
Is there any rationale why we should not simply use Docker Desktop and reinvent the wheel? |
FYI: When I run
This does not interfere with any other WSL distro already installed and always uses the correct WSL distro. |
Also for the record: My test laptop has only 8GB of memory. With kubernetes enabled and all running it shows that 6 GB are used. That is significant but IMHO not much overhead from Docker Desktop at all. |
For #584 (docker & kubernetes support) I improved Windows support and added MacOS support.