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

Update auto_conf.yaml template #1523

Merged
merged 3 commits into from
May 16, 2018
Merged

Update auto_conf.yaml template #1523

merged 3 commits into from
May 16, 2018

Conversation

ChristineTChen
Copy link
Contributor

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Testing Guidelines

An overview on testing
is available in our contribution guidelines.

Versioning

  • Bumped the check version in manifest.json
  • Bumped the check version in datadog_checks/{integration}/__init__.py
  • Updated CHANGELOG.md. Please use Unreleased as the date in the title
    for the new section.
  • If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo

Additional Notes

Anything else we should know when reviewing?


instances:
- url: "%%host%%"
port: "9012"
Copy link
Member

@hkaj hkaj May 9, 2018

Choose a reason for hiding this comment

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

where does this port come from? It's not listed in https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers - is it considered a de facto standard? Does it work in 90%+ of tomcat setups?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The port is also used in the regular tomcat.yaml configuration. But I think the 9012 is the default port tomcat uses for monitoring (https://tomcat.apache.org/tomcat-7.0-doc/monitoring.html)

Copy link
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

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

Can you rebase on master and fix the changelog?

@ChristineTChen ChristineTChen force-pushed the christine/tomcat-auto branch from 93ad090 to ebd2856 Compare May 14, 2018 15:55
@@ -1,5 +1,9 @@
# CHANGELOG - tomcat

# 1.1.0 / Unreleased

* [Update] Update auto_conf template.
Copy link
Contributor

Choose a reason for hiding this comment

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

let's remove the entry from the changelog and put this line in the PR title instead

@ChristineTChen ChristineTChen changed the title [tomcat] Add auto_conf.yaml [tomcat] Add auto_conf.yaml template May 16, 2018
@ChristineTChen ChristineTChen changed the title [tomcat] Add auto_conf.yaml template [tomcat] Update auto_conf.yaml template May 16, 2018
@ChristineTChen ChristineTChen changed the title [tomcat] Update auto_conf.yaml template [Update] Update auto_conf.yaml template May 16, 2018
@ChristineTChen ChristineTChen merged commit 142ff6c into master May 16, 2018
@ChristineTChen ChristineTChen deleted the christine/tomcat-auto branch May 16, 2018 19:52
@rishabh rishabh changed the title [Update] Update auto_conf.yaml template Update auto_conf.yaml template Jun 15, 2018
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.

4 participants