Skip to content

Conversation

@kjohn-msft
Copy link
Collaborator

Handling an issue caught in Canary on auto-assessment code paths:
ERROR:Error: TypeError("run_command_output() missing 1 required positional argument: 'no_output'")

In reviewing this closer, there is also a historical bug in run_command_output CalledProcessError usage where the shadowed class entity (returncode) is used instead of the correct attribute (return_code) in an error log path.

Copilot AI review requested due to automatic review settings May 16, 2025 16:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request addresses a bug encountered in auto-assessment code paths by fixing two issues: ensuring the run_command_output function has an optional no_output parameter and correcting the error logging to use the proper return_code attribute.

  • Updated error logging in both EnvLayer files to reference e.return_code rather than e.returncode.
  • Added a default value (False) for the no_output parameter in run_command_output for improved usability.
  • Included an explicit return of None in get_env_var after error handling.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/extension/src/EnvLayer.py Fixed legacy logging bug using the incorrect attribute (returncode) for CalledProcessError.
src/core/src/bootstrap/EnvLayer.py Provided a default value for no_output in run_command_output and updated error logging.
Comments suppressed due to low confidence (2)

src/core/src/bootstrap/EnvLayer.py:140

  • Providing a default value for no_output changes the function signature; confirm that this modification is compatible with all existing call sites and overall API expectations.
def run_command_output(self, cmd, no_output=False, chk_err=True):

src/core/src/bootstrap/EnvLayer.py:189

  • Verify that using e.return_code for error logging is correct and consistently applied in all error handling paths.
print("Error: CalledProcessError. [Code={0}][Command={1}][Result={2}]".format(str(e.return_code), e.cmd, self.__convert_process_output_to_ascii(e.output[:-1])), file=sys.stdout)

@codecov
Copy link

codecov bot commented May 16, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.77%. Comparing base (bfd698e) to head (4569af7).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/core/src/bootstrap/EnvLayer.py 66.66% 1 Missing ⚠️
src/extension/src/EnvLayer.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #324   +/-   ##
=======================================
  Coverage   93.77%   93.77%           
=======================================
  Files         103      103           
  Lines       17925    17926    +1     
=======================================
+ Hits        16810    16811    +1     
  Misses       1115     1115           
Flag Coverage Δ
python27 93.77% <50.00%> (+<0.01%) ⬆️
python312 93.77% <50.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kjohn-msft kjohn-msft merged commit cf5800f into master May 19, 2025
7 of 8 checks passed
@kjohn-msft kjohn-msft deleted the kjohn-rco branch May 19, 2025 16:36
rane-rajasi added a commit that referenced this pull request May 19, 2025
This release includes:
[x] Bugfix: Fixing Bootstrapper UT failure with Python 2.7: #325 
[x] Bugfix: Optional parameter use in run_command_output & legacy
return_code: #324
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants