raise_for_request should not be used for non-successful response

[stephenmuss]

Hi there,

I'm wondering why you elected to use response.raise_for_status() after receiving the response - https://github.com/okigan/awscurl/blob/184d7735b57d5c71f75f52a85cc8c8909226b6d5/awscurl/awscurl.py#L537

Would it not make more sense to return an exit code that is either 0 for a successful request or non-zero for a non-successful request?

This would mean tools such as jq etc could still parse the response without the Python traceback interfering where a non-successful status code may still be anticipated.

Happy to open a PR if it makes sense.

[okigan]

Thanks for looking into it.

If I recall, the intent was that raise_for_status() would raise an exception which would cause a different return code

I've made this repo to show the intent, check out and run venv and test targets.

Is this working as intended?

[stephenmuss]

Thanks for your quick response @okigan. I understand the intention, the main issue with raise_for_status() is that it raises an exception and will print also print a traceback.

As an example, using awscurl to call a lambda function url with something like the following:

awscurl -H 'Content-Type: application/json' -X POST -d '{"foo": "bar"}' --service lambda --region us-east-1 https://foo.lambda-url.us-east-1.on.aws/

I may expect to get a non-200 status code if the payload is incorrect. Currently with raise_for_status() this looks like

{"errors": {"msg": "Some detailed error description"}}
Traceback (most recent call last):
  File "/Users/steve/.local/share/virtualenvs/example/bin/awscurl", line 8, in <module>
    sys.exit(main())
  File "/Users/steve/.local/share/virtualenvs/example/lib/python3.10/site-packages/awscurl/awscurl.py", line 521, in main
    inner_main(sys.argv[1:])
  File "/Users/steve/.local/share/virtualenvs/example/lib/python3.10/site-packages/awscurl/awscurl.py", line 515, in inner_main
    response.raise_for_status()
  File "/Users/steve/.local/share/virtualenvs/example/lib/python3.10/site-packages/requests/models.py", line 1021, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 422 Client Error: Unprocessable Entity for url: https://foo.lambda-url.us-east-1.on.aws/

Whereas this would be more desirable

{"errors": {"msg": "Some detailed error description"}}

If the main intention was to raise a non-zero exit code could I suggest this instead?

    print(response.text)

    exit_code = 0 if response.ok else 1

    return exit_code

[stephenmuss]

I'm happy to open a PR if you are amenable to that.

[okigan]

Yes, PR would be great!

I had a quick look at response.ok and looks like it also calls raise_for_status which would still produce the output you'd like to avoid? Something similar to this could get the behavior you are looking for.

Again, thanks for looking into this.

[stephenmuss]

Thanks @okigan. I have opened a PR in #153 with an explanation of why response.ok is still sufficient. Thanks for being open to the PR and even more thanks for the very handy tool you've written.