Creating PR with the latest changes #92
Conversation
cloudinary_cli/utils/api_utils.py
Outdated
| if verbose: | ||
| print_json(result) | ||
| uploaded[file_path] = {"path": asset_source(result), "display_path": disp_path} | ||
| if "async" not in options: |
There was a problem hiding this comment.
That is a good catch!
I would change line 127 to:
if "batch_id" in result:
disp_str = f"asynchnously with batch_id: {result['batch_id']}"
else:
disp_str = f"as {result['public_id']}" if not disp_path \
else f"as {disp_path} with public_id: {result['public_id']}"
then you can remove this line (if "async" not in options:)
and in
def asset_source(asset_details):
change to
base_name = asset_details.get('public_id', '')
if not base_name:
return base_name
There was a problem hiding this comment.
This is done.
I had to also update the last line to asset_details.get('format', '') as non-raw files still get the public_id as we specify them so this will prevent no format error as the response won't contain the format
Finally, I updated the display message to say it's processing when uploading asynchronously as well.
cloudinary_cli/modules/clone.py
Outdated
| You need to specify the target cloud via `-t` or `-T` (not both) | ||
| e.g. cld clone -t cloudinary://<api_key>:<api_secret>@<cloudname> -f tags,context -O | ||
| """) | ||
| @option("-T", "--target_saved", |
There was a problem hiding this comment.
We can combine -Tand -t into one. it's easy to check if it's a saved config, or a cloudinary url or not both.
cloudinary_cli/modules/clone.py
Outdated
| help="Tell the CLI the target environemnt to run the command on by specifying a saved configuration - see `config` command.") | ||
| @option("-t", "--target", | ||
| help="Tell the CLI the target environemnt to run the command on by specifying an environment variable.") | ||
| @option("-A", "--auto_paginate", is_flag=True, default=False, |
There was a problem hiding this comment.
This can be removed
cloudinary_cli/modules/clone.py
Outdated
| @option("-F", "--force", is_flag=True, | ||
| help="Skip confirmation.") | ||
| @option("-O", "--overwrite", is_flag=True, default=False, | ||
| help="Skip confirmation.") |
There was a problem hiding this comment.
Doc string seems to be invalid
cloudinary_cli/modules/clone.py
Outdated
| print_help_and_exit() | ||
|
|
||
| base_cloudname_url = os.environ.get('CLOUDINARY_URL') | ||
| base_cloudname = cloudinary.config().cloud_name |
There was a problem hiding this comment.
I would rename base_cloudname to source_cloud_name
cloudinary_cli/modules/clone.py
Outdated
| logger.info("Target environment cannot be the " | ||
| "same as source environment.") | ||
| return True | ||
| refresh_config(base_cloudname_url) |
There was a problem hiding this comment.
Instead of loading target, and then resetting, you can just create a new instance of cloudinary.Config() class and use it.
cloudinary_cli/modules/clone.py
Outdated
| 'secure_url', 'display_name']) | ||
| search.max_results(DEFAULT_MAX_RESULTS) | ||
| res = execute_single_request(search, fields_to_keep="") | ||
| if auto_paginate: |
There was a problem hiding this comment.
auto paginate looks redundant
carlevison
left a comment
carlevison
left a comment
There was a problem hiding this comment.
A few questions and some typo fixes.
cloudinary_cli/modules/clone.py
Outdated
| Cloning restricted assets is also not supported currently. | ||
| Format: cld clone -T <target_environment> <command options> | ||
| `<target_environment>` can be a CLOUDINARY_URL or a saved config (see `config` command) | ||
| e.g. cld clone -T cloudinary://<api_key>:<api_secret>@<cloudname> -f tags,context |
There was a problem hiding this comment.
Should the -f be -fi? It's related to the tag,context fields, right?
| target_config = get_cloudinary_config(target) | ||
| if not target_config: | ||
| logger.error("The specified config does not exist or the CLOUDINARY_URL scheme provided is invalid" | ||
| " (expecting to start with 'cloudinary://').") |
There was a problem hiding this comment.
Does this allow for a saved config?
| help="Specify whether to overwrite existing assets.") | ||
| @option("-w", "--concurrent_workers", type=int, default=30, | ||
| help="Specify the number of concurrent network threads.") | ||
| @option("-fi", "--fields", multiple=True, |
There was a problem hiding this comment.
It would be good to show an example, or state the valid options.
cloudinary_cli/modules/clone.py
Outdated
| Clone assets from one product environment to another with/without tags and/or context (structured metadata is not currently supported). | ||
| Source will be your `CLOUDINARY_URL` environemnt variable but you also can specify a different source using `-c/-C` option. | ||
| Cloning restricted assets is also not supported currently. | ||
| Format: cld clone -T <target_environment> <command options> |
There was a problem hiding this comment.
Is the -T really necessary? You're always going to have to specify a target environment. If you look at the sync command, for example, local_folder and cloudinary_folder are mandatory (no option letter).
cloudinary_cli/modules/clone.py
Outdated
| help="Tell the CLI the target environemnt to run the command on.") | ||
| @option("-f", "--force", is_flag=True, | ||
| help="Skip confirmation.") | ||
| @option("-o", "--overwrite", is_flag=True, default=False, |
There was a problem hiding this comment.
Doesn't -o conflict with the way you specify optional parameters?
Co-authored-by: carlevison <54800761+carlevison@users.noreply.github.com>
Co-authored-by: carlevison <54800761+carlevison@users.noreply.github.com>
Co-authored-by: carlevison <54800761+carlevison@users.noreply.github.com>
Co-authored-by: carlevison <54800761+carlevison@users.noreply.github.com>
Brief Summary of Changes
What does this PR address?
Are tests included?
Reviewer, please note:
Checklist: