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

docs(metrics-addr): Use port 9323, allocated for Docker in prometheus #1589

Merged
merged 1 commit into from
Dec 21, 2018
Merged

docs(metrics-addr): Use port 9323, allocated for Docker in prometheus #1589

merged 1 commit into from
Dec 21, 2018

Conversation

fhemberger
Copy link
Contributor

According to https://github.com/prometheus/prometheus/wiki/Default-port-allocations, 9323 is the default port associated with Docker metrics. It should be used in the example as well to avoid assigning a random port which may interfere with other prometheus exporters on the same machine.

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "patch-1" git@github.com:fhemberger/cli.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@codecov-io
Copy link

Codecov Report

Merging #1589 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1589   +/-   ##
=======================================
  Coverage   55.25%   55.25%           
=======================================
  Files         289      289           
  Lines       19395    19395           
=======================================
  Hits        10716    10716           
  Misses       7983     7983           
  Partials      696      696

1 similar comment
@codecov-io
Copy link

Codecov Report

Merging #1589 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1589   +/-   ##
=======================================
  Coverage   55.25%   55.25%           
=======================================
  Files         289      289           
  Lines       19395    19395           
=======================================
  Hits        10716    10716           
  Misses       7983     7983           
  Partials      696      696

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @fhemberger !

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks! Overall looks good, but I left some suggestions.

Could you address those comments (and amend your commit, so that all changes are in a single commit)?

Based on the branch name (patch-1) I suspect you opened this PR through GitHub's web-interface, and amending a commit requires you to make those changes from the command line, so it's a bit more work to make those changes 😅

Let me know if you want to make those changes yourself, or if you want me to carry those (happy to make them for you)

docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
docs/reference/commandline/dockerd.md Outdated Show resolved Hide resolved
@fhemberger
Copy link
Contributor Author

@thaJeztah Thanks for your feedback, updated the PR. PTAL.

Signed-off-by: Frederic Hemberger <mail@frederic-hemberger.de>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

perfect, LGTM!

thank you so much!

@thaJeztah thaJeztah merged commit 0f33ff0 into docker:master Dec 21, 2018
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Dec 21, 2018
@fhemberger
Copy link
Contributor Author

You're welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants