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

Mongo: Replace Deprecated Parameters #752

Merged
merged 2 commits into from
Oct 4, 2022
Merged

Mongo: Replace Deprecated Parameters #752

merged 2 commits into from
Oct 4, 2022

Conversation

jabbate19
Copy link
Contributor

@jabbate19 jabbate19 commented Sep 13, 2022

What does this PR do?

PyMongo v3.9 and v3.12 Deprecated many ssl parameters in replace of tls counterparts. This was applied to the DataDog/integrations-core repository, but needed to be updated for puppet configs.

Motivation

My DataDog Agent was having trouble running for MongoDB, and after some investigating found it to be due to the Puppet configuration using the older terms.

Additional Notes

For more information on the changes, see DataDog/integrations-core#12743 and https://pymongo.readthedocs.io/en/stable/changelog.html for v3.9 and v3.12

More of a personal note, I need to acknowledge @Mstrodl who pointed out that this issue was connected to the Puppet config! She really helped me find the root of the problem and get around to making these changes!

Describe your test plan

Run this config with an updated DataDog Agent

PyMongo v3.9 and v3.12 Deprecated many ssl parameters in replace of tls
counterparts. This was applied to the DataDog/integrations-core
repository, but needed to be updated for pupper configs.
@jabbate19 jabbate19 requested a review from a team as a code owner September 13, 2022 17:35
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

👋 Apologies for the delay! The new TLS options look good to me, but we have to keep the SSL ones for backwards compatibility for existing users of the Puppet module.

Could you add the SSL options back? We can keep the sample usage section with only the TLS options, but the other two parts (options documentation and template) should still have the SSL options.

Comment on lines 16 to 25
# $ssl
# Optionally enable SSL for connection
# $ssl_ca_certs
# Optionally specify path to SSL Certificate Authority certificates
# $ssl_cert_reqs
# Optionally require SSL client certificate for connection
# $ssl_certfile
# Optionally specify path to SSL certificate for connection
# $ssl_keyfile
# Optionally specify path to SSL private key for connection
Copy link
Member

Choose a reason for hiding this comment

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

Please add these lines back: since the SSL options are still supported they should be documented

@@ -11,19 +11,16 @@ instances:
- <%= tag %>
Copy link
Member

Choose a reason for hiding this comment

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

Please add all the SSL options back on the template too: since they are still supported this may break existing users of the Puppet module, so we should keep them here for now.

@jabbate19 jabbate19 requested a review from mx-psi October 3, 2022 16:44
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

That's perfect, thanks for addressing the comments :)

@mx-psi mx-psi merged commit d4f1e9d into DataDog:main Oct 4, 2022
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.

2 participants