Skip to content

Win32 docs for run-envoy.rst section#15813

Merged
mattklein123 merged 16 commits intoenvoyproxy:mainfrom
davinci26:win32DocsRunEnvoy
Apr 15, 2021
Merged

Win32 docs for run-envoy.rst section#15813
mattklein123 merged 16 commits intoenvoyproxy:mainfrom
davinci26:win32DocsRunEnvoy

Conversation

@davinci26
Copy link
Copy Markdown
Member

Signed-off-by: Sotiris Nanopoulos sonanopo@microsoft.com

Adds a new tab for Windows Containers on the Run Envoy section to illustrate how to use Envoy with a windows container.

Sotiris Nanopoulos added 3 commits April 1, 2021 12:03
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26
Copy link
Copy Markdown
Member Author

@phlax assigning this to you as the docs expert :)

@envoyproxy/windows-dev

Sotiris Nanopoulos added 5 commits April 1, 2021 12:46
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26
Copy link
Copy Markdown
Member Author

@phlax @wrowe can you please take a look when you get some time

@davinci26
Copy link
Copy Markdown
Member Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #15813 (comment) was created by @davinci26.

see: more, trace.

@wrowe
Copy link
Copy Markdown
Contributor

wrowe commented Apr 6, 2021

Looks like you need to push an empty commit here to jog CI

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26
Copy link
Copy Markdown
Member Author

@wrowe and @phlax can you please take a look? This is required for 1.18

@wrowe
Copy link
Copy Markdown
Contributor

wrowe commented Apr 12, 2021

This looks sane to me, but I'm quite unfamiliar with the docs architecture, so @phlax or perhaps @mattklein123 are in a better place to review (it would be handy to have an @envoyproxy/docs team here for such expert review)

@phlax
Copy link
Copy Markdown
Member

phlax commented Apr 12, 2021

i can look over this pr first thing tomorrow

@phlax
Copy link
Copy Markdown
Member

phlax commented Apr 13, 2021

Copy link
Copy Markdown
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

my first impressions are that this significantly complicates this page when (at least atm) the vast majority of users are going to be using a linux container with a linux (or mac, or windows) OS.

This is exacarbated by the complexity of which OS <> which container (i get that you have tried to disambiguate this, but my feeling is that this is still confusing if you dont know in advance what it is trying to communicate)

I dont think the "Windows Service" instructions belong on this page at all. Apart from the fact that this feature is "experimental" - these instructions are for getting started understanding how to specify/use configurations when first starting - nothing to do with running as a service. There are many ways you could run this as a "service" on linux/mac/bsd but there is no way i would want to add example systemd/docker/kubernetes scripts/instructions/commands etc here.

Overall my recommendations would be:

  • remove windows service section
  • rename eg "Docker, Linux/Windows container" to just "Docker (Linux/Windows)" and in the "...(Windows)" tab add a note at the top to say "These are instructions for using the Windows-based image on a Windows OS - see Docker (Linux) if you wish to use the Linux-based image on a Windows OS" - same for each instance of the tab
  • move the note about CON into the windows tab
  • Merge the "System" and "Windows" tab in override - and add "For an equivalent command running on the windows OS..." before the win instructions

@davinci26
Copy link
Copy Markdown
Member Author

Thanks for the review

my first impressions are that this significantly complicates this page when (at least atm) the vast majority of users are going to be using a linux container with a linux (or mac, or windows) OS.

Although this might be the case right now, Windows user should have the same documentation experience. The goal of making Envoy cross platform entails making the documentation cross platform.

IMHO because the examples are under different tabs, the majority of the complication is abstracted away from the user.

rename eg "Docker, Linux/Windows container" to just "Docker (Linux/Windows)" and in the "...(Windows)" tab add a note at the top to say "These are instructions for using the Windows-based image on a Windows OS - see Docker (Linux) if you wish to use the Linux-based image on a Windows OS" - same for each instance of the tab

Windows Container is a standard term that refers to the OS of the container see docs. Whereas Docker (Windows) is ambiguous. With that in mind I prefer the current approach as it clarifies the details.

remove windows service section

I disagree with that. It is a feature that we should document and it is conceptually a different way to register an exe with the system. We should clearly document it as such.

@phlax
Copy link
Copy Markdown
Member

phlax commented Apr 13, 2021

Windows Container is a standard term that refers

i dont think this disambiguates sufficiently.

A container is created from an image - the point was more that it needs a comment in the windows tab - and pref to make the tab titles more succinct.

It is a feature that we should document

not here

@davinci26
Copy link
Copy Markdown
Member Author

Taking a step back, I think the best solution is to use Windows Container and Linux Container and do not mention docket at all.

  1. Docker is not the only container runtime anyway.
  2. It shortens the name
  3. It clarifies the ambiguity regarding the container OS

@davinci26
Copy link
Copy Markdown
Member Author

Regarding the Windows Service, I will take care of it in a follow up PR if you don't mind since this PR is addressing running Envoy on a Windows container

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26
Copy link
Copy Markdown
Member Author

davinci26 commented Apr 14, 2021

The point that I am trying to make is that Docker (Windows) is ambiguous because you don't know if this refers to the host OS or the container OS.

Whereas Docker, Windows Container makes it clear that Windows is the OS of the container.

imho by making the titles more succinct we are losing information.

@phlax
Copy link
Copy Markdown
Member

phlax commented Apr 14, 2021

... the titles more succinct we are losing information.

i think my point is - dont try to disambiguate in the tab title - use the actual tab for that and make it clearer

Docker is not the only container runtime anyway.

no - but these are Docker images - and yep, there are a bunch of different container runtimes that they can be used on.

in this case, and in other places in the docs - the examples use the docker engine as this is the easiest, most x-platform way of trying out these images

Sotiris Nanopoulos added 2 commits April 14, 2021 11:53
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26
Copy link
Copy Markdown
Member Author

davinci26 commented Apr 14, 2021

@phlax followed your suggestion and addressed all your comments

@davinci26
Copy link
Copy Markdown
Member Author

@envoyproxy/senior-maintainers can someone also review this so we can merge it before 1.18 (aka today ❤️)


.. note::

For Linux containers follow the instructions in Docker (Linux) tab and for Windows containers
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

personally i think put this in the actual tabs - dont assume that someone reads the whole doc - also i think it needs to be clearer...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Isnt this repetitive to have this in every tab?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i dont think so - and personally i often ctrl-f or otherwise skip to the section im interested in so i would put it in each tab

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do you think that a green note is distracting away someone who is reading the windows doc tab?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

perhaps dont use an actual .. note: - just some text would do imho

Copy link
Copy Markdown
Member Author

@davinci26 davinci26 Apr 14, 2021

Choose a reason for hiding this comment

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

Shouldn't we just use the title Docker (Windows Container) and Docker (Linux Container) and skip the sentence. We are saving 10 characters from the tab title and then we add two sentences in each tab.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i dont think so - pretty sure this was the conv we already had - not sure what more i can add (other than i also think that image is more appropriate in this context)

--version
...
.. tab:: Docker (Windows)

Copy link
Copy Markdown
Member

@phlax phlax Apr 14, 2021

Choose a reason for hiding this comment

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

like here - i would add:

These are instructions for using the Windows Docker image - if you wish to use the Linux image on a Windows host machine please see the "Docker (Linux)" instructions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this comment placed here is what i think would work best

Sotiris Nanopoulos added 3 commits April 14, 2021 12:32
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@phlax
Copy link
Copy Markdown
Member

phlax commented Apr 15, 2021

@davinci26 can you fix the conflict and i will review the rendered docs

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26
Copy link
Copy Markdown
Member Author

updated

Copy link
Copy Markdown
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

great, thanks @davinci26

@mattklein123 mattklein123 merged commit 5450a45 into envoyproxy:main Apr 15, 2021
douglas-reid pushed a commit to douglas-reid/envoy that referenced this pull request Apr 19, 2021
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Douglas Reid <douglas-reid@users.noreply.github.com>
Monkeyanator pushed a commit to Monkeyanator/envoy that referenced this pull request Apr 20, 2021
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
gokulnair pushed a commit to gokulnair/envoy that referenced this pull request May 6, 2021
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
Signed-off-by: Gokul Nair <gnair@twitter.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants