Adding Windows Subsystem for Linux section to the windows documentation by BalmungSan · Pull Request #1361 · testcontainers/testcontainers-java

BalmungSan · 2019-04-06T19:03:19Z

Resolves #1359

Anyways, English isn't my first language. So, please be carefully when reviewing, specially with grammar mistakes.

kiview

Looks good, thanks!
Just some minor remarks.

Although there are just some little steps required, that should be self-explanatory if you have good experience about Docker for Windows, I think it's a good idea to add this to the docs to make it easier for newcomers.

kiview · 2019-04-07T06:06:50Z

docs/supported_docker_environment/index.md

+| Mac OS X - Docker Toolbox              | Docker Machine v0.8.0       | |
+| Mac OS X - Docker for Mac              | 1.12.0                      | *Support is best-efforts at present*. `getTestHostIpAddress()` is [not currently supported](https://github.com/testcontainers/testcontainers-java/issues/166) due to limitations in Docker for Mac. |
+| Windows - Docker Toolbox               |                             | *Support is limited at present and this is not currently tested on a regular basis*. |
+| Windows - Docker for Windows           | Docker v17.06               | *Support is best-efforts at present.* Only Linux Containers (LCOW) are supported at the moment. See [Windows Support](windows.md). |


Just wondering, why is there a diff in all those other lines?

All these lines had an enormous amount of whitespace characters to fill the last column to make the last pipe (|) align.
They took up to three lines of my console on a 1920 x 1080 monitor.
IMHO, that was simply unnecessary and instead of making the code look more ordered (which cab be said for the first two columns) it made it worse.

kiview · 2019-04-07T06:08:02Z

docs/supported_docker_environment/index.md

+| Mac OS X - Docker for Mac              | 1.12.0                      | *Support is best-efforts at present*. `getTestHostIpAddress()` is [not currently supported](https://github.com/testcontainers/testcontainers-java/issues/166) due to limitations in Docker for Mac. |
+| Windows - Docker Toolbox               |                             | *Support is limited at present and this is not currently tested on a regular basis*. |
+| Windows - Docker for Windows           | Docker v17.06               | *Support is best-efforts at present.* Only Linux Containers (LCOW) are supported at the moment. See [Windows Support](windows.md). |
+| Windows - Windows Substyem for Linux   | Docker v17.06               | *Support is best-efforts at present.* Only Linux Containers (LCOW) are supported at the moment. See [Windows Support](windows.md). |


Suggested change

| Windows - Windows Substyem for Linux | Docker v17.06 | *Support is best-efforts at present.* Only Linux Containers (LCOW) are supported at the moment. See [Windows Support](windows.md). |

| Windows - Windows Substyem for Linux (WSL) | Docker v17.06 | *Support is best-efforts at present.* Only Linux Containers (LCOW) are supported at the moment. See [Windows Support](windows.md). |

Shouldn't it be spelled "Subsystem"?

kiview · 2019-04-07T06:09:34Z

docs/supported_docker_environment/windows.md

+Testcontainers supports communicating with Docker for Windows within the Windows Subsystem for Linux *([**WSL**](https://docs.microsoft.com/en-us/windows/wsl/about))*.
+The following additional configurations steps are required:
+
+ Expose the Docker for windows daemon on the `2375` tcp port without **TLS**.


Suggested change

+ Expose the Docker for windows daemon on the `2375` tcp port without **TLS**.

+ Expose the Docker for Windows daemon on tcp port `2375` without **TLS**.

kiview · 2019-04-07T06:10:12Z

docs/supported_docker_environment/windows.md

+The following additional configurations steps are required:
+
+ Expose the Docker for windows daemon on the `2375` tcp port without **TLS**.
+ Set the `DOCKER_HOST` environment variable as `tcp://localhost:2375`.


Suggested change

+ Set the `DOCKER_HOST` environment variable as `tcp://localhost:2375`.

+ Set the `DOCKER_HOST` environment variable inside the WSL shell to `tcp://localhost:2375`.

kiview · 2019-04-07T06:24:20Z

docs/supported_docker_environment/windows.md

+
+ Expose the Docker for windows daemon on the `2375` tcp port without **TLS**.
+ Set the `DOCKER_HOST` environment variable as `tcp://localhost:2375`.
+ Modify the `/ect/wsl.conf` file to mount the windows drivers on `/` instead of on `/mnt/`.


Suggested change

+ Modify the `/ect/wsl.conf` file to mount the windows drivers on `/` instead of on `/mnt/`.

+ Modify the `/ect/wsl.conf` file to mount the windows filesystem on `/` instead of on `/mnt/`.

I disagree with this one.
Both Docker for Windows and Nick's post refer to these as Drives (I used drivers by mistake, will fix it).

Shall we go with drives then? I don't see too much of a clarity difference, as long as the accidental typo of drivers is fixed.

@rnorth It should have been fixed, as well as all the other comments in 42529b2 & in 37ff88b

I would be happy to fix any other comment you have :)
(however, I deleted the fork due inactivity)

BalmungSan · 2019-04-07T13:20:20Z

Although there are just some little steps required, that should be self-explanatory if you have good experience about Docker for Windows, I think it's a good idea to add this to the docs to make it easier for newcomers.

Care to explain which ones?
I can only see one, and is that for exposing the tcp port 2375 one has to open the settings menu. Will make that explicit.
Apart from that, do you see any other thing that should be made explicit?

stale · 2019-07-07T16:38:10Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

bsideup · 2019-07-19T15:13:56Z

@BalmungSan the PR is pretty hard to review due to the whitespace change. Could you please revert it, and create a follow up PR to only fix the whitespaces later? Thanks!

stale · 2019-10-17T18:21:18Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

rnorth · 2019-10-17T19:03:18Z

It's a shame that this one had become stale - if anybody would like to finish this off (including @BalmungSan) please do so. It'd be good to update this area of the docs.

rnorth · 2020-01-23T19:46:01Z

Replaced by #2282

Adding Windows Subsystem for Linux section to the windows documentation

d3401a7

BalmungSan requested review from bsideup, kiview and rnorth as code owners April 6, 2019 19:03

BalmungSan added 2 commits April 6, 2019 14:08

Fixing typos

a6fd988

Fixing typos

21ef623

kiview requested changes Apr 7, 2019

View reviewed changes

BalmungSan and others added 2 commits April 7, 2019 08:50

Addressing PR comments

42529b2

Fixing typo with Subsystem

37ff88b

stale bot added stale and removed stale labels Jul 7, 2019

bsideup added the type/docs label Jul 19, 2019

stale bot added the stale label Oct 17, 2019

stale bot removed the stale label Oct 17, 2019

rnorth added the help wanted label Oct 17, 2019

rnorth mentioned this pull request Jan 23, 2020

Adding Windows Subsystem for Linux section to the windows documentation #2282

Merged

rnorth closed this Jan 23, 2020

	\| Windows - Windows Substyem for Linux \| Docker v17.06 \| Support is best-efforts at present. Only Linux Containers (LCOW) are supported at the moment. See [Windows Support](windows.md). \|
	\| Windows - Windows Substyem for Linux (WSL) \| Docker v17.06 \| Support is best-efforts at present. Only Linux Containers (LCOW) are supported at the moment. See [Windows Support](windows.md). \|

	+ Expose the Docker for windows daemon on the `2375` tcp port without TLS.
	+ Expose the Docker for Windows daemon on tcp port `2375` without TLS.

	+ Set the `DOCKER_HOST` environment variable as `tcp://localhost:2375`.
	+ Set the `DOCKER_HOST` environment variable inside the WSL shell to `tcp://localhost:2375`.

	+ Modify the `/ect/wsl.conf` file to mount the windows drivers on `/` instead of on `/mnt/`.
	+ Modify the `/ect/wsl.conf` file to mount the windows filesystem on `/` instead of on `/mnt/`.

Uh oh!

Conversation

BalmungSan commented Apr 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiview left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BalmungSan commented Apr 7, 2019

Uh oh!

stale bot commented Jul 7, 2019

Uh oh!

bsideup commented Jul 19, 2019

Uh oh!

stale bot commented Oct 17, 2019

Uh oh!

rnorth commented Oct 17, 2019

Uh oh!

rnorth commented Jan 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

BalmungSan commented Apr 6, 2019 •

edited

Loading