From 03db0b069ffed846f0f02c3a155f7539660b4e2d Mon Sep 17 00:00:00 2001 From: Matthias Dellweg Date: Thu, 13 May 2021 10:50:01 +0200 Subject: [PATCH 1/3] Refactor config command The config command and validation moves to the common section. [noissue] --- pulpcore/cli/common/__init__.py | 45 +++---------------------- pulpcore/cli/{core => common}/config.py | 43 +++++++++++++++++++++-- pulpcore/cli/core/__init__.py | 2 -- 3 files changed, 44 insertions(+), 46 deletions(-) rename pulpcore/cli/{core => common}/config.py (73%) diff --git a/pulpcore/cli/common/__init__.py b/pulpcore/cli/common/__init__.py index ad6e28794..53ee62293 100644 --- a/pulpcore/cli/common/__init__.py +++ b/pulpcore/cli/common/__init__.py @@ -1,13 +1,14 @@ import gettext import os import sys -from typing import Any, Callable, Dict, Optional +from typing import Any, Optional import click import pkg_resources import toml from click_shell import make_click_shell +from pulpcore.cli.common.config import config, config_options, validate_config from pulpcore.cli.common.context import PulpContext from pulpcore.cli.common.debug import debug @@ -20,15 +21,6 @@ PROFILE_KEY = f"{__name__}.profile" -FORMAT_CHOICES = ["json", "yaml", "none"] - - -def _validate_config(config: Dict[str, Any]) -> bool: - if "format" in config and config["format"].lower() not in FORMAT_CHOICES: - raise ValueError(_("'format' is not one of {}").format(FORMAT_CHOICES)) - if "dry_run" in config and not isinstance(config["dry_run"], bool): - raise ValueError(_("'dry_run' is not a bool")) - return True def _config_profile_callback(ctx: click.Context, param: Any, value: Optional[str]) -> Optional[str]: @@ -56,7 +48,7 @@ def _config_callback(ctx: click.Context, param: Any, value: Optional[str]) -> No if PROFILE_KEY in ctx.meta: profile = "cli-" + ctx.meta[PROFILE_KEY] try: - _validate_config(config[profile]) + validate_config(config[profile]) ctx.default_map = config[profile] except KeyError: raise click.ClickException( @@ -70,36 +62,6 @@ def _config_callback(ctx: click.Context, param: Any, value: Optional[str]) -> No raise click.ClickException(_("Aborted.")) -CONFIG_OPTIONS = [ - click.option("--base-url", default="https://localhost", help=_("API base url")), - click.option("--username", help=_("Username on pulp server")), - click.option("--password", help=_("Password on pulp server")), - click.option("--cert", help=_("Path to client certificate")), - click.option( - "--key", - help=_("Path to client private key. Not required if client cert contains this."), - ), - click.option("--verify-ssl/--no-verify-ssl", default=True, help=_("Verify SSL connection")), - click.option( - "--format", - type=click.Choice(FORMAT_CHOICES, case_sensitive=False), - default="json", - help=_("Format of the response"), - ), - click.option( - "--dry-run/--force", - is_flag=True, - help=_("Trace commands without performing any unsafe HTTP calls"), - ), -] - - -def config_options(command: Callable[..., Any]) -> Callable[..., Any]: - for option in reversed(CONFIG_OPTIONS): - command = option(command) - return command - - @click.group() @click.version_option(prog_name=_("pulp3 command line interface")) @click.option( @@ -166,6 +128,7 @@ def _debug_callback(level: int, x: str) -> None: ctx.obj = PulpContext(api_kwargs=api_kwargs, format=format, background_tasks=background) +main.add_command(config) main.add_command(debug) diff --git a/pulpcore/cli/core/config.py b/pulpcore/cli/common/config.py similarity index 73% rename from pulpcore/cli/core/config.py rename to pulpcore/cli/common/config.py index 3357f39cf..dc40f7c73 100644 --- a/pulpcore/cli/core/config.py +++ b/pulpcore/cli/common/config.py @@ -1,17 +1,54 @@ import gettext from pathlib import Path -from typing import Any, Optional +from typing import Any, Callable, Dict, Optional import click import toml -from pulpcore.cli.common import config_options - _ = gettext.gettext LOCATION = str(Path(click.utils.get_app_dir("pulp"), "settings.toml")) +FORMAT_CHOICES = ["json", "yaml", "none"] SETTINGS = ["base_url", "username", "password", "cert", "key", "verify_ssl", "format", "dry_run"] +CONFIG_OPTIONS = [ + click.option("--base-url", default="https://localhost", help=_("API base url")), + click.option("--username", help=_("Username on pulp server")), + click.option("--password", help=_("Password on pulp server")), + click.option("--cert", help=_("Path to client certificate")), + click.option( + "--key", + help=_("Path to client private key. Not required if client cert contains this."), + ), + click.option("--verify-ssl/--no-verify-ssl", default=True, help=_("Verify SSL connection")), + click.option( + "--format", + type=click.Choice(FORMAT_CHOICES, case_sensitive=False), + default="json", + help=_("Format of the response"), + ), + click.option( + "--dry-run/--force", + is_flag=True, + help=_("Trace commands without performing any unsafe HTTP calls"), + ), +] + + +def config_options(command: Callable[..., Any]) -> Callable[..., Any]: + for option in reversed(CONFIG_OPTIONS): + command = option(command) + return command + + +def validate_config(config: Dict[str, Any]) -> bool: + """Validate, that the config provides the proper data types""" + if "format" in config and config["format"].lower() not in FORMAT_CHOICES: + raise ValueError(_("'format' is not one of {}").format(FORMAT_CHOICES)) + if "dry_run" in config and not isinstance(config["dry_run"], bool): + raise ValueError(_("'dry_run' is not a bool")) + return True + @click.group(name="config", help=_("Manage pulp-cli config file")) def config() -> None: diff --git a/pulpcore/cli/core/__init__.py b/pulpcore/cli/core/__init__.py index 8b90db2eb..b00ae23dc 100644 --- a/pulpcore/cli/core/__init__.py +++ b/pulpcore/cli/core/__init__.py @@ -3,7 +3,6 @@ from pulpcore.cli.common import main from pulpcore.cli.core.access_policy import access_policy from pulpcore.cli.core.artifact import artifact -from pulpcore.cli.core.config import config from pulpcore.cli.core.export import export from pulpcore.cli.core.exporter import exporter from pulpcore.cli.core.group import group @@ -23,7 +22,6 @@ # Register commands with cli main.add_command(access_policy) main.add_command(artifact) -main.add_command(config) main.add_command(export) main.add_command(exporter) main.add_command(group) From 0de617be2257fca2573ea0ebf3544be949164e74 Mon Sep 17 00:00:00 2001 From: Matthias Dellweg Date: Thu, 13 May 2021 12:16:54 +0200 Subject: [PATCH 2/3] Unify config validation [noissue] --- pulpcore/cli/common/config.py | 46 +++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/pulpcore/cli/common/config.py b/pulpcore/cli/common/config.py index dc40f7c73..dbe71ba79 100644 --- a/pulpcore/cli/common/config.py +++ b/pulpcore/cli/common/config.py @@ -1,6 +1,6 @@ import gettext from pathlib import Path -from typing import Any, Callable, Dict, Optional +from typing import Any, Callable, Dict, List, Optional import click import toml @@ -41,12 +41,22 @@ def config_options(command: Callable[..., Any]) -> Callable[..., Any]: return command -def validate_config(config: Dict[str, Any]) -> bool: +def validate_config(config: Dict[str, Any], strict: bool = False) -> bool: """Validate, that the config provides the proper data types""" + errors: List[str] = [] if "format" in config and config["format"].lower() not in FORMAT_CHOICES: - raise ValueError(_("'format' is not one of {}").format(FORMAT_CHOICES)) + errors.append(_("'format' is not one of {}").format(FORMAT_CHOICES)) if "dry_run" in config and not isinstance(config["dry_run"], bool): - raise ValueError(_("'dry_run' is not a bool")) + errors.append(_("'dry_run' is not a bool")) + unknown_settings = set(config.keys()) - set(SETTINGS) + if unknown_settings: + errors.append(_("Unknown settings: '{}'.").format("','".join(unknown_settings))) + if strict: + missing_settings = set(SETTINGS) - set(config.keys()) + if missing_settings: + errors.append(_("Missing settings: '{}'.").format("','".join(missing_settings))) + if errors: + raise ValueError("\n".join(errors)) return True @@ -147,18 +157,24 @@ def validate(location: str, strict: bool) -> None: try: settings = toml.load(location) except toml.TomlDecodeError: - raise click.ClickException(f"Invalid toml file '{location}'.") + raise click.ClickException(_("Invalid toml file '{location}'.").format(location=location)) if "cli" not in settings: - raise click.ClickException(f"Could not locate cli section in '{location}'.") - - for setting in settings["cli"]: - if setting not in SETTINGS: - raise click.ClickException(f"Detected unknown setting: '{setting}'.") - - if strict: - for setting in SETTINGS: - if setting not in settings["cli"]: - raise click.ClickException(f"Missing setting: '{setting}'.") + raise click.ClickException(_("Could not locate cli section in '{location}'.").format(location=location)) + + errors: List[str] = [] + for key, profile in settings.items(): + if key != "cli" and not key.startswith("cli-"): + if strict: + errors.append(_("Invalid profile '{}'").format(key)) + continue + try: + validate_config(profile, strict=strict) + except ValueError as e: + errors.append(_("Profile {}:").format(key)) + errors.append(str(e)) + + if errors: + raise click.ClickException("\n".join(errors)) click.echo(f"File '{location}' is a valid pulp-cli config.") From a55545cbe1039aff13b66dbdb301eea29d5fc87a Mon Sep 17 00:00:00 2001 From: Matthias Dellweg Date: Thu, 13 May 2021 12:28:41 +0200 Subject: [PATCH 3/3] Validate settings on edit [noissue] --- CHANGES/239.bugfix | 1 + CHANGES/239.feature | 1 + pulpcore/cli/common/__init__.py | 6 +- pulpcore/cli/common/config.py | 102 +++++++++++++++++--------------- 4 files changed, 59 insertions(+), 51 deletions(-) create mode 100644 CHANGES/239.bugfix create mode 100644 CHANGES/239.feature diff --git a/CHANGES/239.bugfix b/CHANGES/239.bugfix new file mode 100644 index 000000000..4758064cc --- /dev/null +++ b/CHANGES/239.bugfix @@ -0,0 +1 @@ +Properly truncate file before saving settings in ``config edit``. diff --git a/CHANGES/239.feature b/CHANGES/239.feature new file mode 100644 index 000000000..083991147 --- /dev/null +++ b/CHANGES/239.feature @@ -0,0 +1 @@ +Added config validation to ``config create`` and ``config edit``. diff --git a/pulpcore/cli/common/__init__.py b/pulpcore/cli/common/__init__.py index 53ee62293..4be3473f2 100644 --- a/pulpcore/cli/common/__init__.py +++ b/pulpcore/cli/common/__init__.py @@ -8,7 +8,7 @@ import toml from click_shell import make_click_shell -from pulpcore.cli.common.config import config, config_options, validate_config +from pulpcore.cli.common.config import CONFIG_LOCATION, config, config_options, validate_config from pulpcore.cli.common.context import PulpContext from pulpcore.cli.common.debug import debug @@ -37,10 +37,8 @@ def _config_callback(ctx: click.Context, param: Any, value: Optional[str]) -> No if value: config = toml.load(value) else: - default_config_path = os.path.join(click.utils.get_app_dir("pulp"), "settings.toml") - try: - config = toml.load(default_config_path) + config = toml.load(CONFIG_LOCATION) except FileNotFoundError: # No config, but also none requested return diff --git a/pulpcore/cli/common/config.py b/pulpcore/cli/common/config.py index dbe71ba79..37e22b4c1 100644 --- a/pulpcore/cli/common/config.py +++ b/pulpcore/cli/common/config.py @@ -1,23 +1,24 @@ import gettext from pathlib import Path -from typing import Any, Callable, Dict, List, Optional +from typing import Any, Callable, Dict, List, MutableMapping import click import toml _ = gettext.gettext -LOCATION = str(Path(click.utils.get_app_dir("pulp"), "settings.toml")) +CONFIG_LOCATION = str(Path(click.utils.get_app_dir("pulp"), "settings.toml")) FORMAT_CHOICES = ["json", "yaml", "none"] SETTINGS = ["base_url", "username", "password", "cert", "key", "verify_ssl", "format", "dry_run"] CONFIG_OPTIONS = [ click.option("--base-url", default="https://localhost", help=_("API base url")), - click.option("--username", help=_("Username on pulp server")), - click.option("--password", help=_("Password on pulp server")), - click.option("--cert", help=_("Path to client certificate")), + click.option("--username", default="", help=_("Username on pulp server")), + click.option("--password", default="", help=_("Password on pulp server")), + click.option("--cert", default="", help=_("Path to client certificate")), click.option( "--key", + default="", help=_("Path to client private key. Not required if client cert contains this."), ), click.option("--verify-ssl/--no-verify-ssl", default=True, help=_("Verify SSL connection")), @@ -29,7 +30,7 @@ ), click.option( "--dry-run/--force", - is_flag=True, + default=False, help=_("Trace commands without performing any unsafe HTTP calls"), ), ] @@ -60,6 +61,26 @@ def validate_config(config: Dict[str, Any], strict: bool = False) -> bool: return True +def validate_settings(settings: MutableMapping[str, Dict[str, Any]], strict: bool = False) -> bool: + errors: List[str] = [] + if "cli" not in settings: + errors.append(_("Could not locate default profile 'cli' setting")) + + for key, profile in settings.items(): + if key != "cli" and not key.startswith("cli-"): + if strict: + errors.append(_("Invalid profile '{}'").format(key)) + continue + try: + validate_config(profile, strict=strict) + except ValueError as e: + errors.append(_("Profile {}:").format(key)) + errors.append(str(e)) + if errors: + raise ValueError("\n".join(errors)) + return True + + @click.group(name="config", help=_("Manage pulp-cli config file")) def config() -> None: pass @@ -70,7 +91,7 @@ def config() -> None: @click.option("--interactive", "-i", is_flag=True) @click.option("--editor", "-e", is_flag=True, help=_("Edit the config file in an editor")) @click.option("--overwrite", "-o", is_flag=True, help=_("Overwite any existing config file")) -@click.option("--location", default=LOCATION, type=click.Path(resolve_path=True)) +@click.option("--location", default=CONFIG_LOCATION, type=click.Path(resolve_path=True)) @click.pass_context def create( ctx: click.Context, @@ -78,14 +99,7 @@ def create( editor: bool, overwrite: bool, location: str, - base_url: str, - username: Optional[str], - password: Optional[str], - cert: Optional[str], - key: Optional[str], - verify_ssl: bool, - format: str, - dry_run: bool, + **kwargs: Any, ) -> None: def _check_location(loc: str) -> None: if not overwrite and Path(loc).exists(): @@ -100,23 +114,17 @@ def prompt_config_option(name: str) -> Any: help = option.help else: help = option.name - return click.prompt(help, default=(option.default or ""), type=option.type) + return click.prompt(help, default=(option.default), type=option.type) - def _default_value(option: Any) -> Any: - if isinstance(option, bool): - return False - return "" + settings: MutableMapping[str, Any] = kwargs - settings = {} if interactive: - location = click.prompt("Config file location", default=LOCATION) + location = click.prompt("Config file location", default=location) _check_location(location) for setting in SETTINGS: settings[setting] = prompt_config_option(setting) else: _check_location(location) - for setting in SETTINGS: - settings[setting] = locals()[setting] or _default_value(locals()[setting]) output = toml.dumps({"cli": settings}) @@ -124,6 +132,11 @@ def _default_value(option: Any) -> Any: output = click.edit(output) if not output: raise click.ClickException("No output from editor. Aborting.") + try: + settings = toml.loads(output) + validate_settings(settings) + except (ValueError, toml.TomlDecodeError) as e: + raise click.ClickException(str(e)) Path(location).parent.mkdir(parents=True, exist_ok=True) with Path(location).open("w") as sfile: @@ -133,7 +146,7 @@ def _default_value(option: Any) -> Any: @config.command(help=_("Open the settings config file in an editor")) -@click.option("--location", default=LOCATION, type=click.Path(resolve_path=True)) +@click.option("--location", default=CONFIG_LOCATION, type=click.Path(resolve_path=True)) def edit(location: str) -> None: if not Path(location).exists(): raise click.ClickException( @@ -141,17 +154,25 @@ def edit(location: str) -> None: "create command." ) - with Path(location).open("r+") as sfile: - settings = sfile.read() - output = click.edit(settings) + with Path(location).open("r") as sfile: + output = sfile.read() + while True: + output = click.edit(output) if not output: raise click.ClickException("No output from editor. Aborting.") - sfile.seek(0) + try: + settings = toml.loads(output) + validate_settings(settings) + break + except (ValueError, toml.TomlDecodeError) as e: + click.echo(str(e), err=True) + click.confirm(_("Retry"), abort=True) + with Path(location).open("w") as sfile: sfile.write(output) @config.command(help=_("Validate a pulp-cli config file")) -@click.option("--location", default=LOCATION, type=click.Path(resolve_path=True)) +@click.option("--location", default=CONFIG_LOCATION, type=click.Path(resolve_path=True)) @click.option("--strict", is_flag=True, help=_("Validate that all settings are present")) def validate(location: str, strict: bool) -> None: try: @@ -159,22 +180,9 @@ def validate(location: str, strict: bool) -> None: except toml.TomlDecodeError: raise click.ClickException(_("Invalid toml file '{location}'.").format(location=location)) - if "cli" not in settings: - raise click.ClickException(_("Could not locate cli section in '{location}'.").format(location=location)) - - errors: List[str] = [] - for key, profile in settings.items(): - if key != "cli" and not key.startswith("cli-"): - if strict: - errors.append(_("Invalid profile '{}'").format(key)) - continue - try: - validate_config(profile, strict=strict) - except ValueError as e: - errors.append(_("Profile {}:").format(key)) - errors.append(str(e)) - - if errors: - raise click.ClickException("\n".join(errors)) + try: + validate_settings(settings, strict) + except ValueError as e: + raise click.ClickException(str(e)) click.echo(f"File '{location}' is a valid pulp-cli config.")