-
Notifications
You must be signed in to change notification settings - Fork 306
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
Add tests for options on delete #513
Conversation
/azp run DataDog.datadogpy.integration |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Left a note re: the first test failure. The second test failure looks unrelated, we can look into it once the first one is fixed.
/azp run DataDog.datadogpy.integration |
Azure Pipelines successfully started running 1 pipeline(s). |
93a4d5d
to
7c5b7aa
Compare
/azp run DataDog.datadogpy.integration |
Azure Pipelines successfully started running 1 pipeline(s). |
7c5b7aa
to
e814e30
Compare
/azp run DataDog.datadogpy.integration |
Azure Pipelines successfully started running 1 pipeline(s). |
e814e30
to
1763466
Compare
1763466
to
3621c7c
Compare
/azp run DataDog.datadogpy.integration |
Azure Pipelines successfully started running 1 pipeline(s). |
94164de
to
dc6c69a
Compare
/azp run DataDog.datadogpy.integration |
Azure Pipelines successfully started running 1 pipeline(s). |
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 LGTM! Nice work Ian!! ✅
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.
Looks good, once it's fully supported indeed. Left a quick docs note. Thanks!
tests/integration/api/test_api.py
Outdated
)["data"][0] | ||
|
||
# Check if you can delete the monitor with force option | ||
options = {"force": True} |
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. We'll also have to add a note about this options
argument in the docs since it currently mentions it dors not take any JSON args: https://docs.datadoghq.com/api/?lang=python#delete-a-monitor once it's live.
/azp run DataDog.datadogpy.integration |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
No review needs from docs
/azp run DataDog.datadogpy.integration |
Azure Pipelines successfully started running 1 pipeline(s). |
https://datadoghq.atlassian.net/browse/MA-791
What does this PR do?
Add tests to confirm correct behavior of using options with the delete method in the client.
Review checklist (to be filled by reviewers)
changelog/
label attached. If applicable it should have thebackward-incompatible
label attached.do-not-merge/
label attached.kind/
andseverity/
labels attached at least.