-
Notifications
You must be signed in to change notification settings - Fork 260
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
Support DD EU in reporter #468
Conversation
b8f1b1b
to
64a5a41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good. We do break the interface for datadog_agent::reports
currently, let's make it backward compatible.
manifests/reports.pp
Outdated
@@ -15,6 +15,7 @@ | |||
# | |||
class datadog_agent::reports( | |||
$api_key, | |||
$datadog_site, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would not be backward compatible if referenced on its own. It's fine in the context of init.pp
, but if someone has already written a custom manifest using datadog_agent::reports
, this would break it. You'll need to add a default (probably an empty string, or the US site).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, I didn't know it was a public interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a weird edge case that might not affect anyone, but it's not hard to fix on our side, so let's better be safe than sorry. 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now
4ef5507
to
6044e30
Compare
6044e30
to
fa05e3e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! 🍰
* Write api_url in the report config * Use api_url to choose datadog host * Update spec tests * Add a default value for datadog_site
PR to apply on top of #464