Skip to content

Add support for group trigger deletion in hawkular alerts#23

Merged
burmanm merged 1 commit intohawkular:masterfrom
dkorn:delete_group_trigger
Jan 5, 2017
Merged

Add support for group trigger deletion in hawkular alerts#23
burmanm merged 1 commit intohawkular:masterfrom
dkorn:delete_group_trigger

Conversation

@dkorn
Copy link
Copy Markdown
Contributor

@dkorn dkorn commented Dec 21, 2016

Adding delete_group_trigger method in hawkular alerts and test_delete_group_trigger to alerts tests.

Since the DELETE action returns only a response code, without body, I added parse_json flag to _http method in hawkular/client.py. Without this addition an error is raised when trying to load
the json with no content.
I'm setting it to True by default, while sending it with False from the _delete method.
This Issue was solved in hawkular/metrics.py by checking that the method is GET, a wrong solution IMHO cause POST and PUT methods also return body.

@dkorn
Copy link
Copy Markdown
Contributor Author

dkorn commented Dec 21, 2016

@cben @simon3z @rubenvp8510 @enoodle please review

@burmanm
Copy link
Copy Markdown
Contributor

burmanm commented Jan 2, 2017

@dkorn The metrics solution of only GET was usable when the client was created (there were no endpoints that would send response in PUT/POST other than error cases).

Could you rebase this and the other PRs? I wanted to merge the Travis PR before other PRs (for obvious reasons)

Adding delete_group_trigger method in hawkular alerts and
test_delete_group_trigger to alerts tests.

Since the `DELETE` action returns only a response code, without body,
I added `parse_json` flag to `_http` method in `hawkular/client.py`.
Without this addition an error is raised when trying to load
the json with no content. I'm setting it to `True` by default,
while sending it with `False` from the `_delete` method.
This Issue was solved in `hawkular/metrics.py` by checking that the
method is `GET`, a wrong solution IMHO cause `POST` and `PUT` methods
also return body.
@dkorn dkorn force-pushed the delete_group_trigger branch from acefa63 to 52e1edd Compare January 2, 2017 12:37
@dkorn
Copy link
Copy Markdown
Contributor Author

dkorn commented Jan 2, 2017

@dkorn The metrics solution of only GET was usable when the client was created (there were no endpoints that would send response in PUT/POST other than error cases).

I understand, makes sense.

Could you rebase this and the other PRs? I wanted to merge the Travis PR before other PRs (for obvious reasons)

Done.

@burmanm we still need to decide on a solution in cases where there is no response body.
possible solutions:

  1. parse_json flag (as I used here)
  2. read the string with the reader and only if not empty load as JSON
  3. catch an exception (ValueError for example)
  4. other solution

this was also discussed in #28 (review)

@burmanm
Copy link
Copy Markdown
Contributor

burmanm commented Jan 2, 2017

@dkorn I actually like the parse_json flag, not because it is automatic but because it isn't. When we create methods that expect a JSON to be returned, we've defined some sort of agreement. If the JSON wasn't there or it couldn't be parsed, we really should get an error (instead of silent failure).

@dkorn
Copy link
Copy Markdown
Contributor Author

dkorn commented Jan 2, 2017

@burmanm I agree, will also change #28 to include the parse_json flag.
Take into account that put method returns body in some cases, so this flag will be passed through other prior methods.

@rubenvp8510
Copy link
Copy Markdown
Contributor

I reviewed this PR and LGTM, I agree with the change to include the parse_json flag.

@burmanm burmanm merged commit 6ba9af1 into hawkular:master Jan 5, 2017
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.

3 participants