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

Try to be more helpful when JSON gives up #45600

Merged
merged 3 commits into from
Sep 24, 2018
Merged

Conversation

Qalthos
Copy link
Contributor

@Qalthos Qalthos commented Sep 13, 2018

SUMMARY

When bundling params to JSON for ansible-connection, try to be clearer when problems occur about what has gone wrong.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

module_utils/connection.py

ANSIBLE VERSION
2.8

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Sep 13, 2018
"Please open an issue and mention that the culprit is most likely '%s'" % key
)

raise ConnectionError(
Copy link
Member

Choose a reason for hiding this comment

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

You might want to use six.raise_from

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how that helps on Python 2. I don't want to bury the original exception, as that is the one with the details to diagnose an issue, but I also want a more specific message to clue the issue in the right direction.

Copy link
Member

Choose a reason for hiding this comment

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

on the core side we have orig_exec on AnsibleError to allow chaining the exception info, maybe something similar could be added here to the exception objects used?

Copy link
Member

Choose a reason for hiding this comment

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

@Qalthos it won't bury the original exception, it's just a way to spell raise ConnectionError from orig_exc without actually using a syntax, unavailable under Python 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it won't bury the original exception, it's just a way to spell raise ConnectionError from orig_exc without actually using a syntax, unavailable under Python 2.

Maybe I'm missing something, but it does not seem to do anything like that on Python 2

Python 3.7.0 (default, Jul 15 2018, 10:44:58) 
[GCC 8.1.1 20180531] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from ansible.module_utils.six import raise_from
>>> e1 = Exception("Exception 1")
>>> e2 = Exception("Exception 2")
>>> raise_from(e1, e2)
Exception: Exception 2

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 3, in raise_from
Exception: Exception 1
>>> 

This is... fine, it gets the point across and lets me keep the original message while specifying cause.

Python 2.7.15 (default, Jun 27 2018, 13:05:28) 
[GCC 8.1.1 20180531] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from ansible.module_utils.six import raise_from
>>> e1 = Exception("Exception 1")
>>> e2 = Exception("Exception 2")
>>> raise_from(e1, e2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/nate/workspace/ansible/ansible/lib/ansible/module_utils/six/__init__.py", line 744, in raise_from
    raise value
Exception: Exception 1
>>> 

Here, however... e2 is lost, unless you mean to imply that I might want this in addition to adding the original exception text to the new exception which seems redundant in the Python 3 case. Maybe I am completely misunderstanding what you mean, but I don't really see how this helps me.

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 core side we have orig_exec on AnsibleError to allow chaining the exception info, maybe something similar could be added here to the exception objects used?

I had seen that, but couldn't find how it was used. The only thing I can find is https://github.com/bcoca/ansible/blob/devel/bin/ansible#L149. It doesn't look like just popping an orig_exc on to the ConnectionError will trigger that though, and it seems to provide roughly the same functionality as what I am already doing anyway, unless I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

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

@Qalthos exactly, it does nothing under py2, but adds explicit cause under py3. But now that I think about it, it's probably a default behavior anyway.

@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Sep 13, 2018
@ansibot ansibot added support:community This issue/PR relates to code supported by the Ansible community. and removed support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Sep 19, 2018
@Qalthos Qalthos merged commit 03d8fa0 into ansible:devel Sep 24, 2018
@Qalthos Qalthos deleted the helpful-error branch September 24, 2018 13:09
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants