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

Clarify monitor muting endpoints #58

Merged
merged 4 commits into from
Jun 30, 2015
Merged

Clarify monitor muting endpoints #58

merged 4 commits into from
Jun 30, 2015

Conversation

wang-arthur
Copy link
Contributor

  • Adjust dogshell descriptions to distinguish between mute_all/unmute_all and mute/unmute
  • Add new monitor unmute arg (all_scopes) to allow clearing all mute settings for a given monitor
  • Update monitor mute tests
  • Include additional information in 403 response exceptions

@wang-arthur wang-arthur force-pushed the clarify_mute_all branch 3 times, most recently from 05b7caa to dde8ece Compare June 12, 2015 21:06
raise requests.exceptions.HTTPError("{0} ({1})".format(
e.message, error_msg[0]))
except json.JSONDecodeError:
pass
Copy link
Member

Choose a reason for hiding this comment

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

Could you provide further details about the use case for this please ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Some 403 responses include custom error messages. For example, if a user tries to $ dog monitor unmute_all when the mute_all setting isn't enabled, the 403 response includes the error message: 'Monitoring is not currently globally muted'. Simply raising HttpError, however, just gives the user a generic '403 Client forbidden error'.

@yannmh
Copy link
Member

yannmh commented Jun 22, 2015

Thanks alot @wang-arthur. It looks great 👍

Before merging it, I think we should clarify which behaviors fall under the HTTPError orApiError exceptions.

@@ -124,6 +124,14 @@ def request(cls, method, path, body=None, attach_host_name=False, response_forma
if e.response.status_code == 404 or e.response.status_code == 400:
pass
Copy link
Member

Choose a reason for hiding this comment

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

403 should probably be caught in this block and raise at this point https://github.com/DataDog/datadogpy/blob/clarify_mute_all/datadog/api/base.py#L160

Arthur Wang added 4 commits June 30, 2015 17:33
Raising an AppError instead of an HTTPError for 403 responses also
exposes the informational error message in the response.
Command does not add or clear mute settings for individual monitors.
Rather, `mute_all` and `unmute_all` enable/disable a global setting
(also known as downtime).
This argument allows users to clear all muted scopes of a monitor,
instead of unmuting individual scopes one-by-one.
- Add test for unmuting all scopes
- Update outdated mute tests
@wang-arthur
Copy link
Contributor Author

Thanks @yannmh! I made the revisions we discussed and cleaned up the commit history.

@yannmh
Copy link
Member

yannmh commented Jun 30, 2015

Thanks a lot @wang-arthur. Merging it for the v0.7.0 release 🚢

yannmh added a commit that referenced this pull request Jun 30, 2015
Clarify monitor muting endpoints
@yannmh yannmh merged commit bcfccf2 into master Jun 30, 2015
@yannmh yannmh deleted the clarify_mute_all branch June 30, 2015 22:18
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.

2 participants