From 1ba4e41928138604e4db292c652748f39db2063e Mon Sep 17 00:00:00 2001 From: TalLevAmi Date: Sat, 26 Sep 2026 15:36:11 +0000 Subject: [PATCH 1/3] Show a one-line error for an invalid `CLOUDINARY_URL` With `CLOUDINARY_URL=garbage`, every command printed a Python traceback. The SDK reads the variable when it is imported, before `main()` can handle errors. Catch the `ValueError` from `import cloudinary` and exit with one line that tells the user to fix or unset the variable. Co-Authored-By: Claude Opus 5.5 --- cloudinary_cli/__init__.py | 8 +++++++- test/test_cli.py | 12 ++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/cloudinary_cli/__init__.py b/cloudinary_cli/__init__.py index 3d1dcb0..d73c118 100644 --- a/cloudinary_cli/__init__.py +++ b/cloudinary_cli/__init__.py @@ -1,5 +1,11 @@ +import sys + from cloudinary_cli.version import __version__ -import cloudinary +try: + import cloudinary +except ValueError as e: + # The SDK reads CLOUDINARY_URL on import, before the CLI can handle errors. + sys.exit(f"error: {e}. Fix or unset the CLOUDINARY_URL environment variable.") cloudinary.USER_PLATFORM = f"CloudinaryCLI/{__version__}" diff --git a/test/test_cli.py b/test/test_cli.py index f833cc3..43cef5a 100644 --- a/test/test_cli.py +++ b/test/test_cli.py @@ -1,3 +1,6 @@ +import os +import subprocess +import sys import unittest from click.testing import CliRunner @@ -47,3 +50,12 @@ def test_cli_version(self): self.assertIn('Cloudinary CLI', result.output) self.assertIn('Cloudinary SDK', result.output) self.assertIn('Python', result.output) + + def test_invalid_cloudinary_url_env(self): + env = {**os.environ, "CLOUDINARY_URL": "garbage"} + result = subprocess.run([sys.executable, "-m", "cloudinary_cli.cli", "admin", "ping"], + env=env, capture_output=True, text=True) + + self.assertEqual(1, result.returncode) + self.assertIn("Fix or unset the CLOUDINARY_URL environment variable", result.stderr) + self.assertNotIn("Traceback", result.stderr) From 7988b4ba6bc28460d9d3b895166ca0893e21196a Mon Sep 17 00:00:00 2001 From: TalLevAmi Date: Wed, 30 Sep 2026 08:59:26 +0000 Subject: [PATCH 2/3] Let -c/-C override an invalid CLOUDINARY_URL or CLOUDINARY_ACCOUNT_URL The SDK reads both variables on import and raises ValueError when one is invalid. The CLI now imports the SDK without them and then loads each one again. An invalid variable stays unset, so -c and -C can select a config. Without -c or -C, the CLI shows a one-line error that names the variable. --- cloudinary_cli/__init__.py | 26 +++++++++++++++++++------ cloudinary_cli/utils/config_resolver.py | 7 +++++++ test/test_cli.py | 19 ++++++++++++++++++ 3 files changed, 46 insertions(+), 6 deletions(-) diff --git a/cloudinary_cli/__init__.py b/cloudinary_cli/__init__.py index d73c118..23ce0d9 100644 --- a/cloudinary_cli/__init__.py +++ b/cloudinary_cli/__init__.py @@ -1,11 +1,25 @@ -import sys +import os from cloudinary_cli.version import __version__ -try: - import cloudinary -except ValueError as e: - # The SDK reads CLOUDINARY_URL on import, before the CLI can handle errors. - sys.exit(f"error: {e}. Fix or unset the CLOUDINARY_URL environment variable.") +# The SDK reads these variables on import and raises ValueError when one is invalid. Import it without them, +# then load each one again. An invalid one stays unset, so that -c/-C can still select a config. +# resolve_cli_config shows env_config_error when no -c/-C is given. +_env_urls = {name: os.environ.pop(name) for name in ("CLOUDINARY_URL", "CLOUDINARY_ACCOUNT_URL") if name in os.environ} + +import cloudinary +import cloudinary.provisioning + +env_config_error = None +for _name, _reset_config in (("CLOUDINARY_URL", cloudinary.reset_config), + ("CLOUDINARY_ACCOUNT_URL", cloudinary.provisioning.reset_config)): + if _name not in _env_urls: + continue + os.environ[_name] = _env_urls[_name] + try: + _reset_config() + except ValueError as e: + del os.environ[_name] + env_config_error = env_config_error or f"error: {e}. Fix or unset the {_name} environment variable." cloudinary.USER_PLATFORM = f"CloudinaryCLI/{__version__}" diff --git a/cloudinary_cli/utils/config_resolver.py b/cloudinary_cli/utils/config_resolver.py index 38b35ad..ba1c6fb 100644 --- a/cloudinary_cli/utils/config_resolver.py +++ b/cloudinary_cli/utils/config_resolver.py @@ -1,7 +1,10 @@ #!/usr/bin/env python3 +import sys + import cloudinary from click import UsageError, echo +import cloudinary_cli from cloudinary_cli.auth import refresh_url_if_stale from cloudinary_cli.auth.session import strip_oauth_internal_keys from cloudinary_cli.defaults import ( @@ -43,6 +46,10 @@ def resolve_cli_config(config=None, config_saved=None, warn_if_unconfigured=True if config and config_saved: raise UsageError("-c/--config and -C/--config_saved are mutually exclusive; pass only one.") + # An invalid CLOUDINARY_URL/CLOUDINARY_ACCOUNT_URL is an error only when -c/-C does not override it. + if cloudinary_cli.env_config_error and not (config or config_saved): + sys.exit(cloudinary_cli.env_config_error) + cfg = load_config() # -c/-C explicitly select a config; if it is shape-invalid it is incomplete (missing diff --git a/test/test_cli.py b/test/test_cli.py index afdaa81..0fc8a8f 100644 --- a/test/test_cli.py +++ b/test/test_cli.py @@ -60,6 +60,25 @@ def test_invalid_cloudinary_url_env(self): self.assertIn("Fix or unset the CLOUDINARY_URL environment variable", result.stderr) self.assertNotIn("Traceback", result.stderr) + def test_invalid_cloudinary_url_env_with_config_override(self): + env = {**os.environ, "CLOUDINARY_URL": "garbage"} + result = subprocess.run([sys.executable, "-m", "cloudinary_cli.cli", + "-c", "cloudinary://123:abc@demo", "url", "sample"], + env=env, capture_output=True, text=True) + + self.assertEqual(0, result.returncode, result.stderr) + self.assertIn("res.cloudinary.com/demo/image/upload/sample", result.stdout) + + def test_invalid_cloudinary_account_url_env(self): + env = {**os.environ, "CLOUDINARY_ACCOUNT_URL": "garbage"} + env.pop("CLOUDINARY_URL", None) + result = subprocess.run([sys.executable, "-m", "cloudinary_cli.cli", "admin", "ping"], + env=env, capture_output=True, text=True) + + self.assertEqual(1, result.returncode) + self.assertIn("Fix or unset the CLOUDINARY_ACCOUNT_URL environment variable", result.stderr) + self.assertNotIn("Traceback", result.stderr) + def test_unknown_command_suggests_similar(self): result = self.runner.invoke(cli, ['serach', 'cat']) From 9858a9f78058e27374c7c0d7a6c5d41729c085be Mon Sep 17 00:00:00 2001 From: TalLevAmi Date: Wed, 30 Sep 2026 14:08:28 +0000 Subject: [PATCH 3/3] Show the env config error only where the CLI uses the variable Move the SDK import into the new helper `utils/env_config.py`. The helper holds back CLOUDINARY_URL and CLOUDINARY_ACCOUNT_URL during the import. After the import, it wraps `_load_config_from_env` of the SDK config classes. It does not delete the variables from the environment. - The wrapper catches ValueError and TypeError. A failed load keeps no values from the failed load. - The CLOUDINARY_URL error shows only when `resolve_cli_config` falls back to the environment. A saved default, -c and -C override it, and the commands that work without a config (such as `config -n`) ignore it. - The CLOUDINARY_ACCOUNT_URL error shows only in `provisioning`. - The subprocess tests use a temporary CLOUDINARY_HOME. --- cloudinary_cli/__init__.py | 22 +------ cloudinary_cli/core/provisioning.py | 8 +++ cloudinary_cli/utils/config_resolver.py | 13 ++-- cloudinary_cli/utils/env_config.py | 48 +++++++++++++++ test/test_cli.py | 81 +++++++++++++++++++------ 5 files changed, 127 insertions(+), 45 deletions(-) create mode 100644 cloudinary_cli/utils/env_config.py diff --git a/cloudinary_cli/__init__.py b/cloudinary_cli/__init__.py index 23ce0d9..b426e9f 100644 --- a/cloudinary_cli/__init__.py +++ b/cloudinary_cli/__init__.py @@ -1,25 +1,9 @@ -import os - from cloudinary_cli.version import __version__ +from cloudinary_cli.utils.env_config import import_sdk -# The SDK reads these variables on import and raises ValueError when one is invalid. Import it without them, -# then load each one again. An invalid one stays unset, so that -c/-C can still select a config. -# resolve_cli_config shows env_config_error when no -c/-C is given. -_env_urls = {name: os.environ.pop(name) for name in ("CLOUDINARY_URL", "CLOUDINARY_ACCOUNT_URL") if name in os.environ} +# Must run before any other `import cloudinary`, so that an invalid CLOUDINARY_URL does not stop the CLI. +import_sdk() import cloudinary -import cloudinary.provisioning - -env_config_error = None -for _name, _reset_config in (("CLOUDINARY_URL", cloudinary.reset_config), - ("CLOUDINARY_ACCOUNT_URL", cloudinary.provisioning.reset_config)): - if _name not in _env_urls: - continue - os.environ[_name] = _env_urls[_name] - try: - _reset_config() - except ValueError as e: - del os.environ[_name] - env_config_error = env_config_error or f"error: {e}. Fix or unset the {_name} environment variable." cloudinary.USER_PLATFORM = f"CloudinaryCLI/{__version__}" diff --git a/cloudinary_cli/core/provisioning.py b/cloudinary_cli/core/provisioning.py index 92451d3..add5c8d 100644 --- a/cloudinary_cli/core/provisioning.py +++ b/cloudinary_cli/core/provisioning.py @@ -1,7 +1,10 @@ +import sys + from click import command, argument, option import cloudinary.provisioning from cloudinary_cli.utils.api_utils import handle_api_command +from cloudinary_cli.utils.env_config import env_config_error @command("provisioning", @@ -19,6 +22,11 @@ @option("--save", nargs=1, help="Save output to a file.") @option("-d", "--doc", is_flag=True, help="Open the Provisioning API reference in a browser.") def provisioning(params, optional_parameter, optional_parameter_parsed, ls, save, doc): + # -c, -C and the saved default do not replace CLOUDINARY_ACCOUNT_URL, so an invalid one is always an error here. + account_url_error = env_config_error("CLOUDINARY_ACCOUNT_URL") + if account_url_error: + sys.exit(account_url_error) + return handle_api_command(params, optional_parameter, optional_parameter_parsed, ls, save, doc, doc_url="https://cloudinary.com/documentation/provisioning_api", api_instance=cloudinary.provisioning, diff --git a/cloudinary_cli/utils/config_resolver.py b/cloudinary_cli/utils/config_resolver.py index ba1c6fb..d8cbdd7 100644 --- a/cloudinary_cli/utils/config_resolver.py +++ b/cloudinary_cli/utils/config_resolver.py @@ -4,7 +4,6 @@ import cloudinary from click import UsageError, echo -import cloudinary_cli from cloudinary_cli.auth import refresh_url_if_stale from cloudinary_cli.auth.session import strip_oauth_internal_keys from cloudinary_cli.defaults import ( @@ -24,6 +23,7 @@ user_config_names, validate_config_url, ) +from cloudinary_cli.utils.env_config import env_config_error # What the last resolve_cli_config selected, by precedence. One of: # "url" -> an inline -c CLOUDINARY_URL @@ -46,10 +46,6 @@ def resolve_cli_config(config=None, config_saved=None, warn_if_unconfigured=True if config and config_saved: raise UsageError("-c/--config and -C/--config_saved are mutually exclusive; pass only one.") - # An invalid CLOUDINARY_URL/CLOUDINARY_ACCOUNT_URL is an error only when -c/-C does not override it. - if cloudinary_cli.env_config_error and not (config or config_saved): - sys.exit(cloudinary_cli.env_config_error) - cfg = load_config() # -c/-C explicitly select a config; if it is shape-invalid it is incomplete (missing @@ -77,7 +73,12 @@ def resolve_cli_config(config=None, config_saved=None, warn_if_unconfigured=True # No stored default: fall back to the environment. Install it as an OAuthConfig (static, no # saved name -> never refreshes) so the active global is always an OAuthConfig and exposes - # has_oauth uniformly; if nothing is configured, _format_ok warns. + # has_oauth uniformly; if nothing is configured, _format_ok warns. An invalid CLOUDINARY_URL is an + # error only here, and not for the commands that work without a config (such as `config -n`). + url_error = env_config_error("CLOUDINARY_URL") + if url_error and warn_if_unconfigured: + sys.exit(url_error) + if is_env_configured(): _active_source = "env" from cloudinary_cli.auth.oauth_config import install_env_config diff --git a/cloudinary_cli/utils/env_config.py b/cloudinary_cli/utils/env_config.py new file mode 100644 index 0000000..d21ee26 --- /dev/null +++ b/cloudinary_cli/utils/env_config.py @@ -0,0 +1,48 @@ +#!/usr/bin/env python3 +# Do not import the SDK at the top: import_sdk must run before the first `import cloudinary`. +import os + +_ENV_VARS = ("CLOUDINARY_URL", "CLOUDINARY_ACCOUNT_URL") +_errors = {} + + +def import_sdk(): + """ + Import the SDK so that an invalid CLOUDINARY_URL or CLOUDINARY_ACCOUNT_URL does not stop the CLI. + + The SDK loads these variables on import and raises when one is invalid, so they are held back during + the import. Then each config class loads the environment through a safe wrapper: a failed load leaves + an empty config and records the error, which env_config_error returns when the variable is needed. + """ + held = {name: os.environ.pop(name) for name in _ENV_VARS if name in os.environ} + try: + import cloudinary + import cloudinary.provisioning + finally: + os.environ.update(held) + + _wrap_load_from_env(cloudinary.Config, "CLOUDINARY_URL") + _wrap_load_from_env(cloudinary.provisioning.AccountConfig, "CLOUDINARY_ACCOUNT_URL") + cloudinary.reset_config() + cloudinary.provisioning.reset_config() + + +def env_config_error(name): + """The error message for an invalid environment variable `name`, or None if it loaded.""" + return _errors.get(name) + + +def _wrap_load_from_env(config_class, name): + load_from_env = config_class._load_config_from_env + + def safe_load_from_env(self): + before = dict(self.__dict__) + try: + load_from_env(self) + except (ValueError, TypeError) as e: + # Do not keep the values that loaded before the error. + self.__dict__.clear() + self.__dict__.update(before) + _errors[name] = f"error: {e}. Fix or unset the {name} environment variable." + + config_class._load_config_from_env = safe_load_from_env diff --git a/test/test_cli.py b/test/test_cli.py index 0fc8a8f..48e7ac3 100644 --- a/test/test_cli.py +++ b/test/test_cli.py @@ -1,6 +1,8 @@ +import json import os import subprocess import sys +import tempfile import unittest from click.testing import CliRunner @@ -51,34 +53,73 @@ def test_cli_version(self): self.assertIn('Cloudinary SDK', result.output) self.assertIn('Python', result.output) - def test_invalid_cloudinary_url_env(self): - env = {**os.environ, "CLOUDINARY_URL": "garbage"} - result = subprocess.run([sys.executable, "-m", "cloudinary_cli.cli", "admin", "ping"], - env=env, capture_output=True, text=True) + def _run_cli(self, *args, saved=None, **env_vars): + """Run the CLI in a subprocess with only env_vars set, and a temp CLOUDINARY_HOME with the saved configs.""" + with tempfile.TemporaryDirectory() as home: + if saved: + with open(os.path.join(home, "config.json"), "w") as f: + json.dump(saved, f) + env = {k: v for k, v in os.environ.items() if not k.startswith("CLOUDINARY_")} + env.update(CLOUDINARY_HOME=home, **env_vars) + return subprocess.run([sys.executable, "-m", "cloudinary_cli.cli", *args], + env=env, capture_output=True, text=True) + + def assertUrlOutput(self, result): + self.assertEqual(0, result.returncode, result.stderr) + self.assertIn("res.cloudinary.com/demo/image/upload/sample", result.stdout) + def assertEnvError(self, result, name): self.assertEqual(1, result.returncode) - self.assertIn("Fix or unset the CLOUDINARY_URL environment variable", result.stderr) - self.assertNotIn("Traceback", result.stderr) + self.assertEqual(1, len(result.stderr.strip().splitlines()), result.stderr) + self.assertTrue(result.stderr.strip().endswith(f"Fix or unset the {name} environment variable."), + result.stderr) - def test_invalid_cloudinary_url_env_with_config_override(self): - env = {**os.environ, "CLOUDINARY_URL": "garbage"} - result = subprocess.run([sys.executable, "-m", "cloudinary_cli.cli", - "-c", "cloudinary://123:abc@demo", "url", "sample"], - env=env, capture_output=True, text=True) + def test_invalid_cloudinary_url_env(self): + self.assertEnvError(self._run_cli("url", "sample", CLOUDINARY_URL="garbage"), "CLOUDINARY_URL") - self.assertEqual(0, result.returncode, result.stderr) - self.assertIn("res.cloudinary.com/demo/image/upload/sample", result.stdout) + def test_invalid_cloudinary_url_env_query_keys(self): + # The SDK raises TypeError, not ValueError, on this URL. + result = self._run_cli("url", "sample", CLOUDINARY_URL="cloudinary://1:s@demo?a=1&a[b]=2") + self.assertEnvError(result, "CLOUDINARY_URL") - def test_invalid_cloudinary_account_url_env(self): - env = {**os.environ, "CLOUDINARY_ACCOUNT_URL": "garbage"} - env.pop("CLOUDINARY_URL", None) - result = subprocess.run([sys.executable, "-m", "cloudinary_cli.cli", "admin", "ping"], - env=env, capture_output=True, text=True) + def test_invalid_cloudinary_url_env_with_config_override(self): + self.assertUrlOutput(self._run_cli("-c", "cloudinary://123:abc@demo", "url", "sample", + CLOUDINARY_URL="garbage")) + + def test_invalid_cloudinary_url_env_with_config_saved(self): + self.assertUrlOutput(self._run_cli("-C", "demo", "url", "sample", saved={"demo": "cloudinary://123:abc@demo"}, + CLOUDINARY_URL="garbage")) + + def test_invalid_cloudinary_url_env_with_saved_default(self): + saved = {"demo": "cloudinary://123:abc@demo", "__default__": "demo"} + self.assertUrlOutput(self._run_cli("url", "sample", saved=saved, CLOUDINARY_URL="garbage")) + + def test_invalid_cloudinary_url_env_with_cloud_name(self): + # With CLOUDINARY_CLOUD_NAME set, the SDK ignores CLOUDINARY_URL. + self.assertUrlOutput(self._run_cli("url", "sample", CLOUDINARY_URL="garbage", CLOUDINARY_CLOUD_NAME="demo", + CLOUDINARY_API_KEY="123", CLOUDINARY_API_SECRET="abc")) + + def test_invalid_cloudinary_url_env_config_new(self): + # `config -n` works without a config, so the invalid variable does not block it. + # The ping of the new config fails (fake credentials), which is not the error under test. + result = self._run_cli("config", "-n", "demo", "cloudinary://123:abc@demo", CLOUDINARY_URL="garbage") + self.assertNotIn("CLOUDINARY_URL", result.stderr) + self.assertNotIn("Traceback", result.stderr) - self.assertEqual(1, result.returncode) - self.assertIn("Fix or unset the CLOUDINARY_ACCOUNT_URL environment variable", result.stderr) + def test_invalid_cloudinary_account_url_env(self): + # Only `provisioning` uses CLOUDINARY_ACCOUNT_URL. Other commands continue to the API call, + # which fails with the fake credentials. + result = self._run_cli("admin", "ping", CLOUDINARY_URL="cloudinary://123:abc@demo", + CLOUDINARY_ACCOUNT_URL="garbage") + self.assertNotIn("CLOUDINARY_ACCOUNT_URL", result.stderr) self.assertNotIn("Traceback", result.stderr) + def test_invalid_cloudinary_account_url_env_provisioning(self): + # -c does not replace CLOUDINARY_ACCOUNT_URL. + result = self._run_cli("-c", "cloudinary://123:abc@demo", "provisioning", "users", + CLOUDINARY_ACCOUNT_URL="garbage") + self.assertEnvError(result, "CLOUDINARY_ACCOUNT_URL") + def test_unknown_command_suggests_similar(self): result = self.runner.invoke(cli, ['serach', 'cat'])