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.

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

traefik: add page #1512

Merged
merged 4 commits into from
Oct 9, 2017
Merged

traefik: add page #1512

merged 4 commits into from
Oct 9, 2017

Conversation

CaptainYarb
Copy link
Contributor

@CaptainYarb CaptainYarb commented Oct 4, 2017

Closes #1184


  • The page (if new), does not already exist in the repo.

  • The page (if new), has been added to the correct platform folder:
    common/ if it's common to all platforms, linux/ if it's Linux-specific, and so on.

  • The page has 8 or fewer examples.

  • The PR is appropriately titled:
    <command name>: add page for new pages, or <command name>: <description of changes> for pages being edited

  • The page follows the contributing guidelines

Adds the page for Traefik as requested on tldr-pages#1184
@agnivade agnivade added the new command Issues requesting creation of a new page. label Oct 4, 2017
@agnivade agnivade changed the title Traefik: add page traefik: add page Oct 4, 2017
Copy link
Member

@agnivade agnivade left a comment

Choose a reason for hiding this comment

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

Thank you for the page. Left a few comments.

@@ -0,0 +1,19 @@
# traefik

> Traefik HTTP reverse proxy and load balance.
Copy link
Member

Choose a reason for hiding this comment

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

No need to mention traefik again in the heading. Also typo in balance.

I suggest - A HTTP reverse proxy and load balancer


`traefik --c {{config_file}}.toml`

- Start server with a cluster mode enabled:
Copy link
Member

Choose a reason for hiding this comment

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

remove "a"


`traefik --cluster`

- Start server with web ui enabled:
Copy link
Member

Choose a reason for hiding this comment

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

ui => UI.

Also, the words "Start server" are kinda getting repetitive. Maybe you can just say "Enable web UI".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I think it's important to dictate that the flag I'm referring to, both starts the Traefik server and enables the UI. While it's true that all of my items start the server, it's worth ensuring there is no confusion of altering a configuration to enable the web UI. It's a pretty short list, but a couple flags do not actually start the server.
  2. This mirrors the same language on the nginx page. Nginx is pretty similar in terms of having a small number of args that don't start the server.

Copy link
Member

Choose a reason for hiding this comment

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

It's a pretty short list, but a couple flags do not actually start the server.

I see. Okay then.

Copy link
Contributor Author

@CaptainYarb CaptainYarb left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback. I was so caught up in the formatting that I forgot the important parts. Appreciate your patience!


> A HTTP reverse proxy and load balancer.

- Start server with default config:
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps making it "traefik server" instead of just server? Same with below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was merely matching the same wording used on the similar nginx page. I can adjust it if you'd like...

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok! Let's leave it then 😺


`traefik`

- Start server with custom config file:
Copy link
Member

Choose a reason for hiding this comment

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

with custom config file -> with a custom config file might read better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I stated in your other change, this was mirror to the Nginx page's wording. I'm open to changing it if you feel it's necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Here it reads as better English if it has an article before the noun "config file", so I think this change is still needed. I'll update the nginx page too, if everyone else is ok with this?

@CaptainYarb
Copy link
Contributor Author

I've applied all requested grammar fixes to my pull request. Thanks for the feedback.

@agnivade
Copy link
Member

agnivade commented Oct 9, 2017

@Blazedd - How about this comment of mine ?

@sbrl sbrl merged commit bc71152 into tldr-pages:master Oct 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new command Issues requesting creation of a new page.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants