Skip to content

Conversation

@wangzelin007
Copy link
Member

@wangzelin007 wangzelin007 commented Feb 8, 2024


  1. Change the command tree file encoding to utf-8.
  2. Add --overwrite to support Idempotent operations.

This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally? (pip install wheel==0.30.0 required)
  • My extension version conforms to the Extension version schema

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify src/index.json.

@azure-client-tools-bot-prd
Copy link

azure-client-tools-bot-prd bot commented Feb 8, 2024

️✔️Azure CLI Extensions Breaking Change Test
️✔️Non Breaking Changes

@azure-client-tools-bot-prd
Copy link

Hi @wangzelin007,
Please write the description of changes which can be perceived by customers into HISTORY.rst.
If you want to release a new extension version, please update the version in setup.py as well.

@yonzhan
Copy link
Collaborator

yonzhan commented Feb 8, 2024

CI

@wangzelin007 wangzelin007 merged commit 1c3cc18 into main Feb 8, 2024
if not overwrite:
cmd = ['az', 'storage', 'blob', 'exists', '--container-name', f'{STORAGE_CONTAINER}', '--account-name',
f'{STORAGE_ACCOUNT}', '--name', f'{blob_name}', '--auth-mode', 'login']
result = subprocess.run(cmd, check=True, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
Copy link
Member Author

@wangzelin007 wangzelin007 Feb 8, 2024

Choose a reason for hiding this comment

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

If I use result = subprocess.run(cmd, check=True, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT),
The output of result.stdout is like this::
image
https://dev.azure.com/azclitools/internal/_build/results?buildId=128429&view=logs&j=275f1d19-1bd8-5591-b06b-07d489ea915a&t=3178dee2-26ab-587a-4407-0f5d5c54e1f3&l=82

Comment on lines +75 to +76
print(f"Failed to upload '{whl_file}' to the storage account")
raise
Copy link
Member

Choose a reason for hiding this comment

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

Consider raising the error message as an Exception:

raise Exception(f"Failed to upload '{whl_file}' to the storage account")

'--auth-mode', 'login']
result = subprocess.run(cmd, check=True, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
print(json.loads(result.stdout))
'--auth-mode', 'login', '--overwrite']
Copy link
Member Author

Choose a reason for hiding this comment

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

add --overwrite to support Idempotent operations.

'--auth-mode', 'login', '--overwrite']
result = subprocess.run(cmd, capture_output=True)
if result.returncode != 0:
print(f"Failed to upload '{whl_file}' to the storage account")
Copy link
Member Author

Choose a reason for hiding this comment

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

The raise does not contain whl_file related information, so I added this line to print

if not overwrite:
cmd = ['az', 'storage', 'blob', 'exists', '--container-name', f'{STORAGE_CONTAINER}', '--account-name',
f'{STORAGE_ACCOUNT}', '--name', f'{blob_name}', '--auth-mode', 'login']
result = subprocess.run(cmd, check=True, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
Copy link
Member

Choose a reason for hiding this comment

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

Per https://docs.python.org/3/library/subprocess.html#subprocess.run, stdout=subprocess.PIPE, stderr=subprocess.STDOUT combines both streams into one:

If you wish to capture and combine both streams into one, use stdout=PIPE and stderr=STDOUT instead of capture_output.

Copy link
Member Author

@wangzelin007 wangzelin007 Feb 8, 2024

Choose a reason for hiding this comment

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

If I use result = subprocess.run(cmd, check=True, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT),
The output of result.stdout is like this::
image
https://dev.azure.com/azclitools/internal/_build/results?buildId=128429&view=logs&j=275f1d19-1bd8-5591-b06b-07d489ea915a&t=3178dee2-26ab-587a-4407-0f5d5c54e1f3&l=82
I really want to do this, but I don't know why the output is not normal.

Copy link
Member

@jiasli jiasli Feb 8, 2024

Choose a reason for hiding this comment

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

I can reproduce on Linux:

import subprocess
result = subprocess.run(['az', 'account', 'show'], check=True, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
print(result)

shell=True is causing problem.

https://docs.python.org/3/library/subprocess.html#popen-constructor

On POSIX with shell=True, the shell defaults to /bin/sh. ... If args is a sequence, the first item specifies the command string, and any additional items will be treated as additional arguments to the shell itself. That is to say, Popen does the equivalent of:

Popen(['/bin/sh', '-c', args[0], args[1], ...])
$ man sh
...
           -c               Read commands from the command_string operand instead of from the standard
                            input.  Special parameter 0 will be set from the command_name operand and
                            the positional parameters ($1, $2, etc.)  set from the remaining argument
                            operands.

So account, show are actually passed to the shell as $1, $2, not to az.

Copy link
Member

Choose a reason for hiding this comment

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

shell=True comes from #7263 and breaks the az command on Linux. That's why in #7263 (comment) I mentioned it is better to fully test the script before merging it to main.

result = subprocess.run(cmd, check=True, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
if json.loads(result.stdout)['exists']:
result = subprocess.run(cmd, capture_output=True)
if result.stdout and json.loads(result.stdout)['exists']:
Copy link
Member

Choose a reason for hiding this comment

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

Since check=True is removed, it's better to check result.returncode first.

print(json.loads(result.stdout))
'--auth-mode', 'login', '--overwrite']
result = subprocess.run(cmd, capture_output=True)
if result.returncode != 0:
Copy link
Member

@jiasli jiasli Feb 8, 2024

Choose a reason for hiding this comment

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

We can leverage check=True and catch CalledProcessError instead of checking the exit code manually:

https://docs.python.org/3/library/subprocess.html#subprocess.run

If check is true, and the process exits with a non-zero exit code, a CalledProcessError exception will be raised. Attributes of that exception hold the arguments, the exit code, and stdout and stderr if they were captured.

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

Labels

Auto-Assign Auto assign by bot CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants