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

Add new line & adjust spacing around tags #1075

Merged
merged 5 commits into from
Mar 13, 2018
Merged

Conversation

ckelner
Copy link
Contributor

@ckelner ckelner commented Jan 31, 2018

I've had several customers, most recently T-Mobile, improperly indent user and password beneath tags because of the structure of commented out configuration items. This will lead to an error message that states username and password are null.

What does this PR do?

Adds a new line and adjusts spacing/indent for cassandra comments surrounding tags, user, and password (they could be taking the user and password keys all the way back to the space before the comment character, but this seems to have tripped them up). Hopefully is is somewhat more clear now, I tried to follow the convention of # where a space occurs after the hash and aligning the keys so that they're inline.

Motivation

Have had customers get tripped up by simply removing the single comment character # which aligns user and password beneath tags and causes an error (to which they couldn't figure out since YAML isn't in their wheelhouse and the YAML validates).

Versioning

  • Bumped the check version in manifest.json
  • Bumped the check version in datadog_checks/{integration}/__init__.py
    • Kelner: There is no such file for this integration
  • 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
    • Kelner: @olivielpeau has confirmed not necessary for this change

I've had several customers, most recently T-Mobile, improperly indent `user` and `password` beneath tags because of the structure of commented out configuration items. This will lead to an error message that states `username and password are null`.
@olivielpeau
Copy link
Member

Thanks @ckelner! The yaml looked a bit confusing indeed.

To follow the convention we use in our other yamls, I think this is the indentation we should use here:

instances:
  - host: localhost
    port: 7199
    cassandra_aliasing: true

    # Tags are used to be able to filter and aggregate metrics properly, in all the charts.
    #
    # The 'environment' tag depends on the <cluster_name> in Cassandra configuration:
    #   - if the environment information (eg 'prod' or 'test') is included in the <cluster_name>, just use <cluster_name>.
    #   - otherwise it's good to add the information to the 'environment' tag (eg 'prod-<cluster_name>').
    # This will prevent aggregating metrics matchingly named clusters in distinct environments.
    #
    # The 'datacenter' tag should be the name of the Cassandra data center the node belongs to.
    # tags:
    #   environment: default_environment
    #   datacenter: default_datacenter
      
    # user: username
    # password: password
    # process_name_regex: .*process_name.* # Instead of specifying a host, and port. The agent can connect using the attach api.
                                         # This requires the JDK to be installed and the path to tools.jar to be set below.
    # tools_jar_path: /usr/lib/jvm/java-7-openjdk-amd64/lib/tools.jar # To be set when process_name_regex is set
    # name: cassandra_instance
    # java_bin_path: /path/to/java # Optional, should be set if the agent cannot find your java executable
    # java_options: "-Xmx200m -Xms50m" # Optional, Java JVM options
    # trust_store_path: /path/to/trustStore.jks # Optional, should be set if ssl is enabled
    # trust_store_password: password

The general idea is that, to use a commented-out line, you simply have to remove 2 characters (# and a whitespace ), and the line is indented correctly.

What do you think?

About the versioning: updates to the default configurations require bumps to the check version in the 2 files, and to the changelog. I don't think we need to update docs/the docs team here though.

@ckelner
Copy link
Contributor Author

ckelner commented Feb 1, 2018

Hi @olivielpeau thank you for the direction, I've made the adjustments per your direction.

Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

one last thing, and should be good to go!

# # trust_store_password: password
# tags:
# environment: default_environment
# datacenter: default_datacenter
Copy link
Member

Choose a reason for hiding this comment

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

one last nit: can you add one whitespace of indentation to these 2 lines? :)

# tags:
#   environment: default_environment
#   datacenter: default_datacenter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You've got it @olivielpeau 👍

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.

:shipit:

@masci masci merged commit b9798ee into master Mar 13, 2018
@masci masci deleted the ckelner/comment-spacing branch March 13, 2018 08:37
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.

None yet

3 participants