-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[pdh] Make the admin share configurable #5485
Conversation
datadog_checks_base/datadog_checks/base/checks/win/winpdh_base.py
Outdated
Show resolved
Hide resolved
Codecov Report
|
The IBM was issue is known, there is a temporary fix in #5493 |
Adding |
Co-Authored-By: FlorianVeaux <[email protected]>
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.
Just a subtle potential bug, then this looks good to me! (Nice prose that sheds some light on Windows obscure behaviors. 💡)
pdh_check/setup.py
Outdated
@@ -26,7 +26,7 @@ def get_requirements(fpath): | |||
return f.readlines() | |||
|
|||
|
|||
CHECKS_BASE_REQ = 'datadog_checks_base' | |||
CHECKS_BASE_REQ = 'datadog_checks_base>10.2' |
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.
I assume we are targeting 10.3
and up, but this would also match e.g. 10.2.1
, so we should probably change this to…
CHECKS_BASE_REQ = 'datadog_checks_base>10.2' | |
CHECKS_BASE_REQ = 'datadog_checks_base>=10.3' |
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.
I think that'll have to be in a separate PR until datadog_checks_base is released.
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.
I removed the pin for now to unblock this, we can add the pin back once released.
Co-Authored-By: Florimond Manca <[email protected]>
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.
* Add the option to provide a different administrative share * Add admin_share config option * Bump check base dependency * Add tests for new config option * Fix typo * call initialize_pdh_tests * Allow for empty admin share * Inline rstrip * Update conf.yaml.example * Apply suggestions from code review Co-Authored-By: FlorianVeaux <[email protected]> * Update conf.yaml.example * Update pdh_check/setup.py Co-Authored-By: Florimond Manca <[email protected]> * Remove pin on unreleased base version Co-authored-by: FlorianVeaux <[email protected]> Co-authored-by: Florimond Manca <[email protected]>
This reverts commit 1abcf9a.
* Add the option to provide a different administrative share * Add admin_share config option * Bump check base dependency * Add tests for new config option * Fix typo * call initialize_pdh_tests * Allow for empty admin share * Inline rstrip * Update conf.yaml.example * Apply suggestions from code review Co-Authored-By: FlorianVeaux <[email protected]> * Update conf.yaml.example * Update pdh_check/setup.py Co-Authored-By: Florimond Manca <[email protected]> * Remove pin on unreleased base version Co-authored-by: FlorianVeaux <[email protected]> Co-authored-by: Florimond Manca <[email protected]>
Adds the ability to change the administrative share to connect to on pdh check