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.

Sign up for GitHub

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

Jump to bottom

Add config specs #7318

Merged
merged 2 commits into from
Aug 20, 2020
Merged

Add config specs #7318

merged 2 commits into from
Aug 20, 2020

Conversation

hithwen
Copy link
Contributor

@hithwen hithwen commented Aug 7, 2020

No description provided.

@hithwen hithwen requested review from a team as code owners August 7, 2020 10:42
@codecov
Copy link

codecov bot commented Aug 7, 2020 •
edited
Loading

Codecov Report

Merging #7318 into master will increase coverage by 5.41%.
The diff coverage is n/a.

Impacted Files Coverage Δ
kafka/tests/conftest.py
couch/datadog_checks/couch/__about__.py
...g_checks_base/datadog_checks/base/utils/tagging.py
ssh_check/tests/common.py
hive/tests/common.py
...ks_base/datadog_checks/base/utils/warnings_util.py
http_check/tests/conftest.py
presto/datadog_checks/presto/__init__.py
mesos_master/tests/utils.py
gearmand/datadog_checks/gearmand/__about__.py
... and 865 more

ChristineTChen
ChristineTChen requested changes Aug 7, 2020
View reviewed changes
Copy link
Contributor

@ChristineTChen ChristineTChen left a comment

Choose a reason for hiding this comment

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

Small nit with description wording. LGTM otherwise!

cassandra_nodetool/assets/configuration/spec.yaml Outdated
options:
- name: nodetool
description: |
command or path to nodetool (e.g. /usr/bin/nodetool or docker exec container nodetool)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
command or path to nodetool (e.g. /usr/bin/nodetool or docker exec container nodetool)
The command or path to nodetool (e.g. /usr/bin/nodetool or docker exec container nodetool)

Copy link
Contributor

Choose a reason for hiding this comment

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

docs use "for example," (or if you mean "that is," say that) instead of e.g., please

cassandra_nodetool/datadog_checks/cassandra_nodetool/data/conf.yaml.example Outdated
#
# nodetool: /usr/bin/nodetool
## @param nodetool - string - required
## command or path to nodetool (e.g. /usr/bin/nodetool or docker exec container nodetool)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## command or path to nodetool (e.g. /usr/bin/nodetool or docker exec container nodetool)
## The command or path to nodetool (e.g. /usr/bin/nodetool or docker exec container nodetool)

Copy link
Contributor

Choose a reason for hiding this comment

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

same comment about e.g.

kayayarai
kayayarai requested changes Aug 10, 2020
View reviewed changes
cassandra_nodetool/assets/configuration/spec.yaml Outdated
options:
- name: nodetool
description: |
command or path to nodetool (e.g. /usr/bin/nodetool or docker exec container nodetool)
Copy link
Contributor

Choose a reason for hiding this comment

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

docs use "for example," (or if you mean "that is," say that) instead of e.g., please

cassandra_nodetool/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
cassandra_nodetool/assets/configuration/spec.yaml Outdated
can be overwritten on an instance
Note: Agent v6.11+ on Windows runs as an unprivileged user (`ddagentuser`). That user needs to be granted
access to the nodetool installation directory for the check to work.
The nodetool installation also sets some environment variables (e.g. `CASSANDRA_HOME` and `DSCINSTALLDIR`),
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment about e.g.

cassandra_nodetool/assets/configuration/spec.yaml Outdated Show resolved Hide resolved
cassandra_nodetool/assets/configuration/spec.yaml Outdated
Note: Agent v6.11+ on Windows runs as an unprivileged user (`ddagentuser`). That user needs to be granted
access to the nodetool installation directory for the check to work.
The nodetool installation also sets some environment variables (e.g. `CASSANDRA_HOME` and `DSCINSTALLDIR`),
but sets them as variables only for the user doing the nodetool installation. Those environment variables should
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean "You should also set those environment variables as system-wide variables."? That's more direct than saying they "should be set."

cassandra_nodetool/datadog_checks/cassandra_nodetool/data/conf.yaml.example Outdated
## Host that the Datadog Cassandra Nodetool check connects to.
#
# host: localhost

## @param port - integer - optional
## @param port - integer - optional - default: 7199
## The port JMX is listening to for connections.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## The port JMX is listening to for connections.
## The port JMX listens to for connections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is code generated

cassandra_nodetool/datadog_checks/cassandra_nodetool/data/conf.yaml.example Show resolved Hide resolved
cassandra_nodetool/datadog_checks/cassandra_nodetool/data/conf.yaml.example Show resolved Hide resolved
cassandra_nodetool/datadog_checks/cassandra_nodetool/data/conf.yaml.example Show resolved Hide resolved
cassandra_nodetool/datadog_checks/cassandra_nodetool/data/conf.yaml.example Show resolved Hide resolved
@hithwen @kayayarai
Apply suggestions from code review
26925ba 
Co-authored-by: Kari Halsted <12926135+kayayarai@users.noreply.github.com>
@hithwen hithwen force-pushed the julia/cassandra_nodetool_config_specs branch from 573b4e3 to 26925ba Compare August 18, 2020 13:18
@hithwen hithwen requested a review from ChristineTChen August 19, 2020 08:16
@hithwen hithwen merged commit 73bd64b into master Aug 20, 2020
@hithwen hithwen deleted the julia/cassandra_nodetool_config_specs branch August 20, 2020 12:28
github-actions bot pushed a commit that referenced this pull request Aug 20, 2020
@hithwen
Add config specs (#7318)
034a60c 
* Add config specs

Co-authored-by: Kari Halsted <12926135+kayayarai@users.noreply.github.com> 73bd64b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kayayarai kayayarai kayayarai requested changes

@ChristineTChen ChristineTChen ChristineTChen approved these changes

Assignees
No one assigned
Labels
documentation integration/cassandra_nodetool
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hithwen @kayayarai @ChristineTChen