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

Remove utils that are available in datadog_checks #3750

Merged
merged 4 commits into from
Jul 19, 2018

Conversation

nmuesch
Copy link
Contributor

@nmuesch nmuesch commented Jun 6, 2018

Note: Please remember to review the Datadog Contribution Guidelines
if you have not yet done so.

What does this PR do?

Remove a lot of code that is already available in datadog_checks_base.

Motivation

Keep only one set of code to make things easier to update.

Testing Guidelines

An overview on testing
is available in our contribution guidelines.

Additional Notes

The way the imports work on the datadog_checks lib means that in order to test the wmi checks, it requires the win32 extension to be available. So we now conditionally import the wmi_calculator stuff so that it will pass if it doesn't exist. This should cause the tests to fail if we are on a windows machine, but be properly skipped if we aren't (had to do this to get Travis to be 🆗 )

This PR assumes this is in datadog-checks-base DataDog/integrations-core#1897

@nmuesch nmuesch force-pushed the nick/remove_redundant_libs branch from 6e300dc to 6422847 Compare June 9, 2018 17:42
@nmuesch nmuesch force-pushed the nick/remove_redundant_libs branch from f5356e9 to 044bd54 Compare June 11, 2018 17:16
@nmuesch nmuesch requested a review from a team June 11, 2018 17:37
Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

Well done!

@olivielpeau olivielpeau self-requested a review June 18, 2018 22:09
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.

reviewed very quickly, didn't notice anything wrong 👍 I'll ask for a second pair of eyes from croissant.

Do you have a plan to test these changes?

@nmuesch
Copy link
Contributor Author

nmuesch commented Jul 12, 2018

@olivielpeau I was able to build the Agent and tested the imports continue to work on Windows and Linux specifically. Could someone from croissant take another pass? Would love to get this merged for the next minor release.

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.

LGTM!

This PR should be safe to merge because Nick has been able to confirm that:

  • all checks in 5.25 are already importing from these utils from datadog_checks_base
  • except wmi_check, which has been updated on master, and for which Nick's double checked the related wmi utils in datadog_checks_base are up-to-date with the ones in dd-agent

Copy link
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@truthbk truthbk merged commit 51b0621 into master Jul 19, 2018
@truthbk truthbk deleted the nick/remove_redundant_libs branch July 19, 2018 08:54
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.

4 participants