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

Make tornado raise error configurable #32942

Merged
merged 1 commit into from
Apr 29, 2016
Merged

Conversation

onorua
Copy link
Contributor

@onorua onorua commented Apr 29, 2016

What does this PR do?

Let us disable "default" tornado behavior upon request in case non 200 response is returned. By default it raise an error without checking for body of the error.

What issues does this PR fix or reference?

Systems like kubernetes provide description of error message while returning non 200 status code. Example of this message is:

status: 400
body: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"asafdlakj in version \"v1\" cannot be handled as a ReplicationController: no kind \"asafdlakj\" is registered for version \"v1\"","reason":"BadRequest","code":400}

while with default raising error for the same POST query, I can got only:

        error:
            HTTP 400: Bad Request
        status:
            400

which makes impossible to create human friendly error messaging. This blocks me to push complete k8s module, as one moths of usage on our environment proved that it is impossible to manage production systems without proper logging of errors.

@@ -132,6 +132,7 @@ def query(url,
handle=False,
agent=USERAGENT,
hide_fields=None,
raise_error=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this default need to be set to True? Seems like that will trigger a behavior change here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the contrary, since v4.1.0 it supports this parameter to overwrite default value which is True, also documentation for current stable tornado confirms it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah perfect. That's just what I was wondering. Thanks for confirming.

@rallytime rallytime merged commit 3828837 into saltstack:develop Apr 29, 2016
gitebra pushed a commit to gitebra/salt that referenced this pull request Apr 29, 2016
* commit '12a74c24a370e8473bd7b386a911be11577db41c':
  Cisco NSO Proxy Minion (saltstack#32911)
  Add caching loader, with msgpack module (saltstack#32953)
  Make tornado raise error configurable (saltstack#32942)
@onorua
Copy link
Contributor Author

onorua commented May 27, 2016

@rallytime 2016.3 is released, but I don't see the change :( When is it going to be available?

@rallytime
Copy link
Contributor

@onorua This change is not available in the 2016.3.0 release because it was submitted against the develop branch. The "feature release" of 2016.3 was cut from develop a couple of months (beginning of February) before this change was submitted. As it stands right now, this change will be available in the next feature release, which is codenamed Carbon.

The process for submitting code and an explanation of which branch submissions should be made against can be found here, but basically this breaks down to "Bugs should be submitted against the earliest supported release branch that the bug appears on, and features should be submitted against develop". Does that help?

@onorua
Copy link
Contributor Author

onorua commented Jun 1, 2016

@rallytime will it help if I raise a bug, and close it with PR for 2016.3 branch?

rallytime pushed a commit to rallytime/salt that referenced this pull request Jun 1, 2016
@rallytime
Copy link
Contributor

@onorua I have back-ported this to 2016.3 in #33680.

@rallytime rallytime added the ZZZ[Done]-back-ported-bf RETIRED The pull request has been back-ported to an older branch. label Jun 1, 2016
cachedout pushed a commit that referenced this pull request Jun 1, 2016
@onorua
Copy link
Contributor Author

onorua commented Jun 3, 2016

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ZZZ[Done]-back-ported-bf RETIRED The pull request has been back-ported to an older branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants