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.

Sign up for GitHub

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

Jump to bottom

Do not use str() on exceptions #46950

Merged
merged 3 commits into from Nov 9, 2018
Merged

Do not use str() on exceptions #46950

merged 3 commits into from Nov 9, 2018

Conversation

mkrizek
Copy link
Contributor

@mkrizek mkrizek commented Oct 12, 2018

SUMMARY
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

many

ANSIBLE VERSION
devel
ADDITIONAL INFORMATION

@mkrizek mkrizek requested a review from abadger October 12, 2018 13:59
@mkrizek mkrizek changed the title Use to_native on exceptions Do not use str() on exceptions Oct 12, 2018
@ansibot
Copy link
Contributor

ansibot commented Oct 12, 2018

Hi @mkrizek, thank you for submitting this pull-request!

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Oct 12, 2018

cc @Akasurde @NilashishC @Qalthos @davixx @ganeshrn @gdpak @jpmens @justjais @rcarrillocruz @rrey @sfromm @trishnaguha @wisotzky
click here for bot help

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_maintainer Ansibot is unable to identify maintainers for this PR. (Check `author` in docs or BOTMETA.yml) needs_triage Needs a first human triage before being processed. networking Network category support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Oct 12, 2018
@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Oct 12, 2018
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Oct 25, 2018
@mkrizek mkrizek requested a review from webknjaz November 1, 2018 09:59
bcoca
bcoca approved these changes Nov 1, 2018
View reviewed changes
Copy link
Member

@bcoca bcoca left a comment

Choose a reason for hiding this comment

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

lgtm, just 1 that is 'on the fence', left comment on that line, not a blocker

abadger
abadger reviewed Nov 1, 2018
View reviewed changes
lib/ansible/modules/source_control/git.py Show resolved Hide resolved
lib/ansible/modules/system/sysctl.py Outdated Show resolved Hide resolved
lib/ansible/plugins/connection/paramiko_ssh.py Outdated Show resolved Hide resolved
lib/ansible/plugins/connection/paramiko_ssh.py Outdated Show resolved Hide resolved
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 2, 2018
@mkrizek mkrizek requested a review from abadger November 2, 2018 09:05
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 2, 2018
@abadger
Copy link
Contributor

abadger commented Nov 8, 2018

I made some comments because I'm nitpicky but this is fine to merge with or without those changes applied :-) Good work!

@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 8, 2018
@mkrizek mkrizek merged commit a80c25c into ansible:devel Nov 9, 2018
@mkrizek mkrizek deleted the to_native branch November 9, 2018 06:59
Ghilli3 pushed a commit to Ghilli3/ansible that referenced this pull request Nov 12, 2018
@mkrizek @Ghilli3
Do not use str() on exceptions (ansible#46950)
28393ae
mjmayer pushed a commit to mjmayer/ansible that referenced this pull request Nov 30, 2018
@mkrizek @mjmayer
Do not use str() on exceptions (ansible#46950)
607f4d4
Tomorrow9 pushed a commit to Tomorrow9/ansible that referenced this pull request Dec 4, 2018
@mkrizek @Tomorrow9
Do not use str() on exceptions (ansible#46950)
83e922c
@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.
Reviewers

@abadger abadger abadger left review comments

@webknjaz webknjaz webknjaz approved these changes

@bcoca bcoca bcoca approved these changes

Assignees
No one assigned
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_maintainer Ansibot is unable to identify maintainers for this PR. (Check `author` in docs or BOTMETA.yml) networking Network category support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mkrizek @ansibot @abadger @webknjaz @bcoca