Why not just override request() here? At least I believe get() and post() call it.

Can you write a test for this? I.e. if there is a retry failure when fetching gating.yaml, a human readable error should be passed to client (there was a bug report for this in past).

Why not just override request() here? At least I believe get() and post() call it.

Good point, thx

Can you write a test for this? I.e. if there is a retry failure when fetching gating.yaml, a human readable error should be passed to client (there was a bug report for this in past).

Done

Can you override directly? The use of a wrapper function seems unnecessary.

Remove redundant line.

Single underscore prefix means that the variable is protected -- should be only set in subclasses of the response.

Single underscore prefix means that the variable is protected -- should be only set in subclasses of the response.

I know that. But I didn't find any other way to set a response content. Because content property doesn't have a setter.

Can you override directly? The use of a wrapper function seems unnecessary.

Done

I know that. But I didn't find any other way to set a response content. Because content property doesn't have a setter.

How about just create a new response class as mentioned here? Or subclass Response and override content property.

Or just move the whole session requests business into simple request function -- on most places it's handled the same way:
- make a request
- raise for status
- get JSON data

I know that. But I didn't find any other way to set a response content. Because content property doesn't have a setter.

How about just create a new response class as mentioned here? Or subclass Response and override content property.
Or just move the whole session requests business into simple request function -- on most places it's handled the same way:
- make a request
- raise for status
- get JSON data

Created a subclass

You can write this also as:
url_arg = kwargs.get('url', args[1])

But are we sure args[1] is always there? I'm worried that IndexError could be raised here.

You can write this also as:
url_arg = kwargs.get('url', args[1])
But are we sure args[1] is always there? I'm worried that IndexError could be raised here.

Good point. Thank you. Added.
There should be as long as 'url' is a mandatory argument in request function. So if this argument is missed TypeError will be thrown normally.

Awesome, thank you!
The PR looks good to me +1
Let's wait for @lholecek final review, then it's good to go.

I liked the generic response from your previous patch better. This submodule shouldn't know about any details from other submodules.

If there needs to be a custom error message passed to client, it should be handled by the code which requests the gating.yaml.

Shouldn't this also include urllib3.exceptions.MaxRetryError?

I noticed some of these exceptions you're handling are handled in json_error: https://pagure.io/greenwave/blob/master/f/greenwave/utils.py#_30

Any reason why those are kept there or why you are moving away from this?

Do you think it would work better to override the property in this subclass? I'm bit skeptical that _content won't break in some future requests releases (I don't think it's a documented attribute).

@property
def content(self):
    return self.__error_message

Shouldn't this also include urllib3.exceptions.MaxRetryError?

MaxRetryError is always being reraised by one of those five errors. See requests/adapters.py line 500. The logic here is that MaxRetryError is not an error itself. It is always caused by something else (like we've reached MaxRetryError cause we were unable to connect).

Probably better not to check these calls. The details/arguments can change often, forcing us to update tests.

I noticed some of these exceptions you're handling are handled in json_error: https://pagure.io/greenwave/blob/master/f/greenwave/utils.py#_30
Any reason why those are kept there or why you are moving away from this?

Only one exception is the same there. It's a ConnectionError. Maybe I could remove it safely but I'm not sure if it wouldn't also cover something else. It always better to have two exception handlers instead of none.

Probably better not to check these calls. The details/arguments can change often, forcing us to update tests.

Removed

Do you think it would work better to override the property in this subclass? I'm bit skeptical that _content won't break in some future requests releases (I don't think it's a documented attribute).
@property
def content(self):
return self.__error_message

Done

This is no longer covered by tests and I expect it's now handled in greenwave/request_session.py. Is that correct? If so, remove this conditional branch.

+1