Skip to content

Conversation

@evelyn-ys
Copy link
Member

@evelyn-ys evelyn-ys commented Dec 13, 2021

Description

Feature asked by #20662 #20663 #20666
This PR:

  • Adds positional argument extra_options with experimental tag for flexibility
  • Upgrades azcopy from 10.8.0 to 10.13.0
  • Removes preview tag for az storage copy

Testing Guide

az storage copy -s source_file -d destination_container_url -- --block-size-mb=0.25 --check-length

History Notes

[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change.
[Component Name 2] az command b: Add some customer-facing feature.


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

@evelyn-ys evelyn-ys self-assigned this Dec 13, 2021
@evelyn-ys evelyn-ys added this to the Dec 2021 (2022-01-04) milestone Dec 13, 2021
@jiasli
Copy link
Member

jiasli commented Dec 13, 2021

Unfortunately, there is no easy solution for argument pass-though. Apparently, you are choosing the 1st workaround from #18869. This makes passing quotes " ' complicated as they need to be quoted in shell, such as

--extra-options "--some-other-parameter \"value with space\""

or even worse with single quotes both inside and outside

--extra-options '--some-other-parameter '"'"'value with space'"'"' something_else'
                 ^^^^^^^^^^^^^^^^^^^^^^^  ^  ^^^^^^^^^^^^^^^^  ^  ^^^^^^^^^^^^^^^  # string content

$ echo '--some-other-parameter '"'"'value with space'"'"' something_else'
--some-other-parameter 'value with space' something_else

$ echo '--some-other-parameter "value with \"quote\""'
--some-other-parameter "value with \"quote\""

$ echo "--some-other-parameter \"value with \\\"quote\\\"\""
--some-other-parameter "value with \"quote\""

@jiasli
Copy link
Member

jiasli commented Dec 13, 2021

Things in PowerShell get worse as now you need to consider escaping 3 times from

  1. az's shlex which is Bash syntax
  2. CMD
  3. PowerShell

This becomes unmanageably complicated:

> az "--some-other-param \`"value with \\\`"quote\\\`"\`"" --debug
cli.knack.cli: Command arguments: ['--some-other-param "value with \\"quote\\""', '--debug']

I don't want to explain how this works. 😣

See https://github.com/Azure/azure-cli/blob/dev/doc/quoting-issues-with-powershell.md

@jiasli
Copy link
Member

jiasli commented Dec 13, 2021

Here is the script I use to debug argument passing in shells:

# echo_args.py

import sys


def echo(args):
    print('[str]')
    for i, arg in enumerate(args):
        print(f"{i}: {arg!s}")

    print()

    print('[repr]')
    for i, arg in enumerate(args):
        print(f"{i}: {arg!r}")


echo(sys.argv)

You can invoke it by

> python .\echo_args.py --extra-options "--some-other-param \`"value with \\\`"quote\\\`"\`""
[str]
0: .\echo_args.py
1: --extra-options
2: --some-other-param "value with \"quote\""

[repr]
0: '.\\echo_args.py'
1: '--extra-options'
2: '--some-other-param "value with \\"quote\\""'

@evelyn-ys evelyn-ys changed the title [Storage] az storage copy: Add --extra-options to pass through options to azcopy [Storage] az storage copy: Add positional argument extra_options to pass through options to azcopy Dec 15, 2021
@jiasli
Copy link
Member

jiasli commented Dec 15, 2021

Copying my comment from #20663 (comment)

This topic was intensively discussed before in #18869.

Then I would propose the -- solution:

az storage copy -s source_file -d container_url -- --bla="bla" --foo="foo" --bar="bar"

This is a common convention in Linux world:

@chasewilson for awareness. If we go down this path, then other commands can do the same.

@chasewilson
Copy link
Contributor

Copying my comment from #20663 (comment)

This topic was intensively discussed before in #18869.

Then I would propose the -- solution:

az storage copy -s source_file -d container_url -- --bla="bla" --foo="foo" --bar="bar"

This is a common convention in Linux world:

@chasewilson for awareness. If we go down this path, then other commands can do the same.

I think this makes a lot of sense all things considered. I'm glad this is experimental as I'm hesitent to make this the standard for similar scenarios. From a command line perspective, it's less than ideal to have a "native" command like this where you have to know another command line tools (azcopy) parameters to make the most of it.

Is there are reason we don't support wrapping the new flags and parameters. Please forgive me if i'm catching up and I'm not saying we shouldn't implement the current solution, I just want to understand all avenues before committing.

Another question, are we relying on the native bash parsing for the -- mangagement? If so, how will we approach from Windows?

Thank you!

@jiasli
Copy link
Member

jiasli commented Dec 16, 2021

Another question, are we relying on the native bash parsing for the -- mangagement? If so, how will we approach from Windows?

-- interpretation is provided by Python's built-in lib argparse, so yes, it works on Windows too.

@evelyn-ys
Copy link
Member Author

Is there are reason we don't support wrapping the new flags and parameters.

There has been and will be more and more azcopy options in new versions. It's kind of duplicated work for CLI to add all the options and release new version. So we would like pass-through arguments. But if you think it's safer to wrap them all, I can also understand.

@evelyn-ys evelyn-ys requested a review from jiasli December 27, 2021 02:11
@evelyn-ys evelyn-ys merged commit 5cf240d into Azure:dev Dec 27, 2021
MichaelWhi pushed a commit to MichaelWhi/azure-cli that referenced this pull request Apr 19, 2023
evelyn-ys pushed a commit that referenced this pull request Apr 20, 2023
…ns to pass through options to azcopy. similar to #20702 (#26127)

Co-authored-by: Michael Whittaker <[email protected]>
avgale pushed a commit to avgale/azure-cli that referenced this pull request Aug 24, 2023
…ns to pass through options to azcopy. similar to Azure#20702 (Azure#26127)

Co-authored-by: Michael Whittaker <[email protected]>
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