-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
reuse python code from datadog_checks_base #1704
Conversation
@ofek: could you explain a little more what this PR does and why ? |
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.
Made a few inline comments, overall LGTM.
A few general comments/questions:
- can you squash your commits?
- can you add a
reno
release note with adeprecations
section mentioning that these python import paths should be changed (in user-provided custom checks)? - Is there a plan to do a similar change in Agent5? If not I think we should also list this deprecation in
docs/agent/changes.md
def report_as_service_check(self, sc_name, status, instance, msg=None): | ||
"""This function should be implemented by inherited classes""" | ||
raise NotImplementedError | ||
from datadog_checks.checks import NetworkCheck, Status |
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.
let's expose EventType
too; any reason not to?
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.
It's not used anywhere. Shall I add it anyway?
|
||
from prometheus_check import * | ||
from . functions import parse_metric_family # noqa: F401 | ||
from datadog_checks.checks.prometheus import PrometheusCheck |
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 we should also import PrometheusFormat
and UnknownFormatError
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.
They are not used anywhere. Shall I add them anyway?
@@ -174,16 +174,31 @@ def misspell(ctx, targets): | |||
print("misspell found no issues") | |||
|
|||
@task | |||
def deps(ctx): | |||
def deps(ctx, no_checks=False, core_dir=None, verbose=False): |
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.
Can you document how a developer is supposed to use these options when setting up their dev environment? Could be documented in the command help, might be worth updating the docs/dev/
if applicable.
|
||
if not no_checks: | ||
verbosity = 'v' if verbose else 'q' | ||
core_dir = core_dir or os.getenv('DD_CORE_DIR') |
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.
DD_CORE_DIR
should be documented
@ofek about the legacy imports that I mentioned in my review: the rationale is to continue supporting the old import paths for custom checks that may still be using them, so I think we should do that for every class/function that could reasonably be imported from a custom check. Ideally, once all the integrations-core checks use the new import paths, we'd log a deprecation warning when an old import path is used so that it's clear to users that they need to update their custom checks' code. |
- | | ||
The core Agent check Python code is no longer duplicated here and is instead | ||
pulled from integrations-core. The code now resides in the `datadog_checks` | ||
namespace, though the old `checks`, `utils`, etc. paths are still supported. |
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.
can you add something like: Please update your custom checks accordingly
, and add a link to the relevant documentation?
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.
LGTM except one nit on the docs
docs/dev/agent_dev_env.md
Outdated
@@ -27,6 +27,40 @@ You must install [go](https://golang.org/doc/install) version 1.9.2 or above. Ma | |||
sure that `$GOPATH/bin` is in your `$PATH` otherwise Invoke cannot use any | |||
additional tool it might need. | |||
|
|||
### Python |
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 Python
section mostly talks about installing the dev versions of python needed to build the agent against. So I think it should be left below in the System or Embedded
section.
Here, you could simply mention in the Invoke
section that a standard python+pip install is required for development maybe? What do you think?
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.
Nice! 👌
This removes all code that is also in https://github.com/DataDog/integrations-core/tree/master/datadog_checks_base by simply importing what is necessary.
Motivation
Deduplication of logic: We often have to update code in both places at the same time.
depends on:
DataDog/integrations-core#1561 ✔️
DataDog/integrations-core#1562 ✔️
DataDog/integrations-core#1565 ✔️
DataDog/integrations-core#1566 ✔️