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

Always ensure_unicode for subprocess output #2941

Merged
merged 2 commits into from
Jan 15, 2019
Merged

Conversation

nmuesch
Copy link
Collaborator

@nmuesch nmuesch commented Jan 14, 2019

What does this PR do?

Remove unnecessary if branches for ensuring unicode around the output from subprocess.

Motivation

If the response from subprocess is empty, python3 will return a byte and not be converted to unicode, which can break some checks that don't require subproccess_out returns a non empty value.

Review checklist

  • PR has a meaningful title or PR has the no-changelog label attached
  • Feature or bugfix has tests
  • Git history is clean
  • If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo
  • If PR adds a configuration option, it has been added to the configuration file.

Additional Notes

Anything else we should know when reviewing?

@nmuesch nmuesch requested review from a team as code owners January 14, 2019 01:41
@nmuesch nmuesch changed the title Always ensure unicode Always ensure_unicode for out/err Jan 14, 2019
@nmuesch nmuesch changed the title Always ensure_unicode for out/err Always ensure_unicode for subprocess output Jan 14, 2019
@codecov-io
Copy link

codecov-io commented Jan 14, 2019

Codecov Report

Merging #2941 into master will decrease coverage by 9.16%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2941      +/-   ##
==========================================
- Coverage    84.7%   75.54%   -9.17%     
==========================================
  Files         659       45     -614     
  Lines       37508     3570   -33938     
  Branches     4482      427    -4055     
==========================================
- Hits        31772     2697   -29075     
+ Misses       4421      764    -3657     
+ Partials     1315      109    -1206

if err:
err = ensure_unicode(err)
out = ensure_unicode(out)
err = ensure_unicode(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need to check for None ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, lets. The subprocess command seems to actually come from some binding in the Agent, so better to be safer than sorry here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants