Skip to content

{CI} Merge CLI extension release scripts from release-scripts branch to main branch #7273

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

Merged
wangzelin007 merged 7 commits into from
Feb 8, 2024
Merged

{CI} Merge CLI extension release scripts from release-scripts branch to main branch #7273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2517e69
update
wangzelin007 Feb 7, 2024
bc3aed8
update
wangzelin007 Feb 7, 2024
f2b7bab
update
wangzelin007 Feb 7, 2024
87f519a
update
wangzelin007 Feb 7, 2024
8906e0b
update
wangzelin007 Feb 7, 2024
b6017d4
Update update_ext_cmd_tree.py
wangzelin007 Feb 7, 2024
2c3d7a3
Update update_ext_cmd_tree.py
wangzelin007 Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 25 additions & 13 deletions scripts/ci/sync_extensions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,20 +62,27 @@ def _sync_wheel(ext, updated_indexes, failed_urls, overwrite, temp_dir):
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 •
edited
Loading

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

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.

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 •
edited
Loading

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 •
edited
Loading

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

@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.

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.

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

@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.

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

print("Skipping '{}' as it already exists...".format(whl_file))
return

cmd = ['az', 'storage', 'blob', 'upload', '--container-name', f'{STORAGE_CONTAINER}', '--account-name',
f'{STORAGE_ACCOUNT}', '--name', f'{blob_name}', '--file', f'{os.path.abspath(whl_path)}',
'--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

@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.

add --overwrite to support Idempotent operations.

result = subprocess.run(cmd, capture_output=True)
if result.returncode != 0:
Copy link
Member

@jiasli jiasli Feb 8, 2024 •
edited
Loading

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.

print(f"Failed to upload '{whl_file}' to the storage account")
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.

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

raise
Comment on lines +75 to +76
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.

Consider raising the error message as an Exception:

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

cmd = ['az', 'storage', 'blob', 'url', '--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)
url = json.loads(result.stdout)
result = subprocess.run(cmd, capture_output=True)
print(result)
if result.stdout and result.returncode == 0:
url = json.loads(result.stdout)
else:
print("Failed to get the URL for '{}'".format(whl_file))
raise
updated_index = ext
updated_index['downloadUrl'] = url
updated_indexes.append(updated_index)
Expand Down Expand Up @@ -148,9 +155,12 @@ def main():
# backup the old index.json
backup_index_name = f'{BLOB_PREFIX}/index.json.sav' if BLOB_PREFIX else 'index.json.sav'
cmd = ['az', 'storage', 'blob', 'upload', '--container-name', f'{STORAGE_CONTAINER}', '--account-name',
f'{STORAGE_ACCOUNT}', '--name', f'{backup_index_name}', '--file', f'{os.path.abspath(target_index_path)}', '--auth-mode', 'login']
result = subprocess.run(cmd, check=True, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
print(json.loads(result.stdout))
f'{STORAGE_ACCOUNT}', '--name', f'{backup_index_name}',
'--file', f'{os.path.abspath(target_index_path)}', '--auth-mode', 'login', '--overwrite']
result = subprocess.run(cmd, capture_output=True)
if result.returncode != 0:
print(f"Failed to upload '{target_index_path}' to the storage account")
raise
# start with an empty index.json to sync all extensions
initial_index = {"extensions": {}, "formatVersion": "1"}
open(target_index_path, 'w').write(json.dumps(initial_index, indent=4, sort_keys=True))
Expand All @@ -174,9 +184,11 @@ def main():
index_name = f'{BLOB_PREFIX}/index.json' if BLOB_PREFIX else 'index.json'
cmd = ['az', 'storage', 'blob', 'upload', '--container-name', f'{STORAGE_CONTAINER}', '--account-name',
f'{STORAGE_ACCOUNT}', '--name', f'{index_name}', '--file', f'{os.path.abspath(target_index_path)}',
'--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']
result = subprocess.run(cmd, capture_output=True)
if result.returncode != 0:
print(f"Failed to upload '{target_index_path}' to the storage account")
raise
print("\nSync finished.")
if updated_indexes:
print("New extensions available in:")
Expand Down
19 changes: 13 additions & 6 deletions scripts/ci/update_ext_cmd_tree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def update_cmd_tree(ext_name):
sys.path.append(ext_dir)
extension_command_table, _ = _load_extension_command_loader(invoker.commands_loader, None, ext_mod)

EXT_CMD_TREE_TO_UPLOAD = Session()
EXT_CMD_TREE_TO_UPLOAD = Session(encoding='utf-8')
EXT_CMD_TREE_TO_UPLOAD.load(os.path.expanduser(os.path.join('~', '.azure', file_name)))
root = {}
for cmd_name, ext_cmd in extension_command_table.items():
Expand Down Expand Up @@ -81,14 +81,21 @@ def upload_cmd_tree():
file_path = os.path.expanduser(os.path.join('~', '.azure', file_name))

cmd = ['az', 'storage', 'blob', 'upload', '--container-name', f'{STORAGE_CONTAINER}', '--account-name',
f'{STORAGE_ACCOUNT}', '--name', f'{blob_file_name}', '--file', f'{file_path}', '--auth-mode', 'login']
result = subprocess.run(cmd, check=True, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
print(json.loads(result.stdout))
f'{STORAGE_ACCOUNT}', '--name', f'{blob_file_name}', '--file', f'{file_path}', '--auth-mode', 'login',
'--overwrite']
result = subprocess.run(cmd, capture_output=True)
if result.returncode != 0:
print(f"Failed to upload '{blob_file_name}' to the storage account")
print(result)

cmd = ['az', 'storage', 'blob', 'url', '--container-name', f'{STORAGE_CONTAINER}', '--account-name',
f'{STORAGE_ACCOUNT}', '--name', f'{blob_file_name}', '--auth-mode', 'login']
result = subprocess.run(cmd, check=True, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
url = json.loads(result.stdout)
result = subprocess.run(cmd, capture_output=True)
if result.stdout and result.returncode == 0:
url = json.loads(result.stdout)
else:
print(f"Failed to get the URL for '{blob_file_name}'")
raise

download_file_path = os.path.expanduser(os.path.join('~', '.azure', downloaded_file_name))
download_file(url, download_file_path)
Expand Down