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

[jmx] add rmi registry ssl config option #2371

Merged
merged 1 commit into from
Oct 11, 2018
Merged

Conversation

arbll
Copy link
Member

@arbll arbll commented Oct 10, 2018

What does this PR do?

Add a few new SSL related config option introduced in DataDog/jmxfetch#185

Review checklist

  • PR has a meaningful title or PR has the no-changelog label attached
  • Git history is clean
  • 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?

Copy link
Contributor

@nmuesch nmuesch left a comment

Choose a reason for hiding this comment

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

This looks good to me overall. Were you able to test out this feature with one of these integrations? (Asking since we don't have integration tests for the JMX checks)

Looks like two of these integrations conf.yaml files were moved to the new format, for readability. Can you adjust those as well?

I also think we should make this a changelog/added label as it means the conf yaml file being shipped is going to contain new params

@arbll
Copy link
Member Author

arbll commented Oct 11, 2018

Thanks for the feedback @nmuesch! The new features are linked to the target JVM configuration and were tested against a simple Java app.

I fixed the two conflicts and moved them to the new format.

Copy link
Contributor

@nmuesch nmuesch left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for this and confirming it works on your test target JVM!

@nmuesch nmuesch merged commit 386cb64 into master Oct 11, 2018
@masci masci deleted the arbll/rmi-ssl-jmxfetch branch December 17, 2018 14:10
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.

None yet

2 participants