From 9baea1c6b8fc0cc5c4354cde59d2cac6dca3f70c Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Wed, 19 Jun 2024 13:07:30 +0200 Subject: [PATCH 1/3] =?UTF-8?q?=E2=9C=A8=20CLI:=20Make=20`NON=5FINTERACTIV?= =?UTF-8?q?E`=20option=20a=20switch=20instead=20of=20flag?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For many setup/configuration CLI commands, the `NON_INTERACTIVE` option is added to allow the user to run the command without being prompted for input and use the defaults instead. However, new users are often not aware of this option, and will not understand some options as they are prompted. Even when a sensible default is offered by the prompt, users will still want to understand the option and be unsure if the default works for them. Hence, it might be preferable to run it non-interactively by default for some commands. Here we adapt the `NON_INTERACTIVE` option into a switch (`-n/-I` or `--non-interactive`/`--interactive`) that is `--interactive` by default. --- docs/source/reference/command_line.rst | 51 +++++++++++-------- .../cmdline/params/options/interactive.py | 4 +- src/aiida/cmdline/params/options/main.py | 12 +++-- 3 files changed, 40 insertions(+), 27 deletions(-) diff --git a/docs/source/reference/command_line.rst b/docs/source/reference/command_line.rst index 0953d027f7..b3edb33a0e 100644 --- a/docs/source/reference/command_line.rst +++ b/docs/source/reference/command_line.rst @@ -329,23 +329,26 @@ Below is a list with all available subcommands. the newly created profile uses the new PostgreSQL database instead of SQLite. Options: - --profile-name TEXT Name of the profile. By default, a unique name starting with - `presto` is automatically generated. [default: (dynamic)] - --email TEXT Email of the default user. [default: (dynamic)] - --use-postgres When toggled on, the profile uses a PostgreSQL database - instead of an SQLite one. The connection details to the - PostgreSQL server can be configured with the relevant options. - The command attempts to automatically create a user and - database to use for the profile, but this can fail depending - on the configuration of the server. - --postgres-hostname TEXT The hostname of the PostgreSQL server. - --postgres-port INTEGER The port of the PostgreSQL server. - --postgres-username TEXT The username of the PostgreSQL user that is authorized to - create new databases. - --postgres-password TEXT The password of the PostgreSQL user that is authorized to - create new databases. - -n, --non-interactive Never prompt, such as for sudo password. - --help Show this message and exit. + --profile-name TEXT Name of the profile. By default, a unique name starting + with `presto` is automatically generated. [default: + (dynamic)] + --email TEXT Email of the default user. [default: (dynamic)] + --use-postgres When toggled on, the profile uses a PostgreSQL database + instead of an SQLite one. The connection details to the + PostgreSQL server can be configured with the relevant + options. The command attempts to automatically create a + user and database to use for the profile, but this can + fail depending on the configuration of the server. + --postgres-hostname TEXT The hostname of the PostgreSQL server. + --postgres-port INTEGER The port of the PostgreSQL server. + --postgres-username TEXT The username of the PostgreSQL user that is authorized + to create new databases. + --postgres-password TEXT The password of the PostgreSQL user that is authorized + to create new databases. + -n, --non-interactive / -I, --interactive + Never prompt, such as for sudo password. [default: + (--interactive)] + --help Show this message and exit. .. _reference:command-line:verdi-process: @@ -412,8 +415,11 @@ Below is a list with all available subcommands. (Deprecated) Setup a new profile in a fully automated fashion. Options: - -n, --non-interactive In non-interactive mode, the CLI never prompts but - simply uses default values for options that define one. + -n, --non-interactive / -I, --interactive + In non-interactive mode, the CLI never prompts for + options but simply uses default values for options that + define one. In interactive mode, the CLI will prompt for + each interactive option. [default: (--interactive)] --profile PROFILE The name of the new profile. [required] --email EMAIL Email address associated with the data you generate. The email address is exported along with the data, when @@ -516,8 +522,11 @@ Below is a list with all available subcommands. user has been created. Options: - -n, --non-interactive In non-interactive mode, the CLI never prompts but - simply uses default values for options that define one. + -n, --non-interactive / -I, --interactive + In non-interactive mode, the CLI never prompts for + options but simply uses default values for options that + define one. In interactive mode, the CLI will prompt for + each interactive option. [default: (--interactive)] --profile PROFILE The name of the new profile. [required] --email EMAIL Email address associated with the data you generate. The email address is exported along with the data, when diff --git a/src/aiida/cmdline/params/options/interactive.py b/src/aiida/cmdline/params/options/interactive.py index c044d04907..d6c216eca9 100644 --- a/src/aiida/cmdline/params/options/interactive.py +++ b/src/aiida/cmdline/params/options/interactive.py @@ -167,9 +167,9 @@ def get_default(self, ctx: click.Context, call: bool = True) -> t.Optional[t.Uni def is_interactive(ctx: click.Context) -> bool: """Return whether the command is being run non-interactively. - This is the case if the ``non_interactive`` parameter in the context is set to ``True``. + This is the case if the ``non_interactive`` parameter in the context is set to ``False``. - :return: ``True`` if being run non-interactively, ``False`` otherwise. + :return: ``True`` if being run interactively, ``False`` otherwise. """ return not ctx.params.get('non_interactive', False) diff --git a/src/aiida/cmdline/params/options/main.py b/src/aiida/cmdline/params/options/main.py index 85b3090ad5..aa86a1f0dd 100644 --- a/src/aiida/cmdline/params/options/main.py +++ b/src/aiida/cmdline/params/options/main.py @@ -344,11 +344,15 @@ def set_log_level(_ctx, _param, value): ) NON_INTERACTIVE = OverridableOption( - '-n', - '--non-interactive', - is_flag=True, + '-n/-I', + '--non-interactive/--interactive', is_eager=True, - help='In non-interactive mode, the CLI never prompts but simply uses default values for options that define one.', + help=( + 'In non-interactive mode, the CLI never prompts for options but simply uses default values for options that ' + 'define one. In interactive mode, the CLI will prompt for each interactive option. ' + ), + default=False, + show_default='--interactive', ) DRY_RUN = OverridableOption('-n', '--dry-run', is_flag=True, help='Perform a dry run.') From ce38e91a0dde9d07c4709bbd21b206c61f0f7d4a Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Wed, 19 Jun 2024 13:09:12 +0200 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=91=8C=20CLI:=20Make=20`configure-rab?= =?UTF-8?q?bitmq`=20non-interactive=20by=20default?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `configure-rabbitmq` command was introduced mainly to allow new users that set up their profile with `verdi presto` before they set up RabbitMQ to upgrade their profile to use the message broker. However, since the command is interactive, they would be prompted for each of the options when running the command without `-n`/`--non-interactive`. Users that don't understand these inputs will typically want to set the defaults, so switching the modus operandi of the commmand to be non-interactive will make life easier for these users. Users that _do_ want to set different values than the defaults will understand the options of the command and should be able to be able to provide them directly or via the interactive mode. --- src/aiida/cmdline/commands/cmd_profile.py | 2 +- tests/cmdline/commands/test_profile.py | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/aiida/cmdline/commands/cmd_profile.py b/src/aiida/cmdline/commands/cmd_profile.py index 0b8065a9a8..55d59f706e 100644 --- a/src/aiida/cmdline/commands/cmd_profile.py +++ b/src/aiida/cmdline/commands/cmd_profile.py @@ -136,7 +136,7 @@ def profile_setup(): @setup.SETUP_BROKER_HOST() @setup.SETUP_BROKER_PORT() @setup.SETUP_BROKER_VIRTUAL_HOST() -@options.NON_INTERACTIVE() +@options.NON_INTERACTIVE(default=True, show_default='--non-interactive') @click.pass_context def profile_configure_rabbitmq(ctx, profile, **kwargs): """Configure RabbitMQ for a profile. diff --git a/tests/cmdline/commands/test_profile.py b/tests/cmdline/commands/test_profile.py index 51594b6ca7..9718bd06f5 100644 --- a/tests/cmdline/commands/test_profile.py +++ b/tests/cmdline/commands/test_profile.py @@ -287,6 +287,13 @@ def test_configure_rabbitmq(run_cli_command, isolated_config): run_cli_command(cmd_profile.profile_configure_rabbitmq, options, use_subprocess=False) assert profile.process_control_backend == 'core.rabbitmq' + # Verify that running in non-interactive mode is the default + options = [ + profile_name, + ] + run_cli_command(cmd_profile.profile_configure_rabbitmq, options, use_subprocess=True) + assert profile.process_control_backend == 'core.rabbitmq' + # Call it again to check it works to reconfigure existing broker connection parameters options = [profile_name, '-n', '--broker-host', 'rabbitmq.broker.com'] run_cli_command(cmd_profile.profile_configure_rabbitmq, options, use_subprocess=False) From a49e79afb061d403b383facadfc53c1d98b8fde8 Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Wed, 19 Jun 2024 13:26:55 +0200 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=91=8C=20CLI:=20Give=20feedback=20for?= =?UTF-8?q?=20`configure-rabbitmq`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently the `verdi profile configure-rabbitmq` command doesn't give any feedback to the user whether the provided options can successfully connect to the RabbitMQ server. Here we adapt the `detect_rabbitmq_config` function to accept the broker configuration as `**kwargs`, and use it to check if the provided options in the `configure-rabbitmq` can successfully connect to the RabbitMQ server. A "success" message is printed if we can connect to the server, else a warning is printed and the user is asked for confirmation before proceeding to configure the broker. A `--force` flag is also added to avoid asking for confirmation in case the command is unable to connect to the broker. --- src/aiida/brokers/rabbitmq/defaults.py | 24 +++++++++++++++-------- src/aiida/cmdline/commands/cmd_profile.py | 18 ++++++++++++++++- tests/cmdline/commands/test_profile.py | 18 ++++++++++++----- 3 files changed, 46 insertions(+), 14 deletions(-) diff --git a/src/aiida/brokers/rabbitmq/defaults.py b/src/aiida/brokers/rabbitmq/defaults.py index aeeaab578d..b312897f73 100644 --- a/src/aiida/brokers/rabbitmq/defaults.py +++ b/src/aiida/brokers/rabbitmq/defaults.py @@ -29,7 +29,15 @@ ) -def detect_rabbitmq_config() -> dict[str, t.Any] | None: +def detect_rabbitmq_config( + protocol: str | None = None, + username: str | None = None, + password: str | None = None, + host: str | None = None, + port: int | None = None, + virtual_host: str | None = None, + heartbeat: int | None = None, +) -> dict[str, t.Any] | None: """Try to connect to a RabbitMQ server with the default connection parameters. :returns: The connection parameters if the RabbitMQ server was successfully connected to, or ``None`` otherwise. @@ -37,13 +45,13 @@ def detect_rabbitmq_config() -> dict[str, t.Any] | None: from kiwipy.rmq.threadcomms import connect connection_params = { - 'protocol': os.getenv('AIIDA_BROKER_PROTOCOL', BROKER_DEFAULTS['protocol']), - 'username': os.getenv('AIIDA_BROKER_USERNAME', BROKER_DEFAULTS['username']), - 'password': os.getenv('AIIDA_BROKER_PASSWORD', BROKER_DEFAULTS['password']), - 'host': os.getenv('AIIDA_BROKER_HOST', BROKER_DEFAULTS['host']), - 'port': os.getenv('AIIDA_BROKER_PORT', BROKER_DEFAULTS['port']), - 'virtual_host': os.getenv('AIIDA_BROKER_VIRTUAL_HOST', BROKER_DEFAULTS['virtual_host']), - 'heartbeat': os.getenv('AIIDA_BROKER_HEARTBEAT', BROKER_DEFAULTS['heartbeat']), + 'protocol': protocol or os.getenv('AIIDA_BROKER_PROTOCOL', BROKER_DEFAULTS['protocol']), + 'username': username or os.getenv('AIIDA_BROKER_USERNAME', BROKER_DEFAULTS['username']), + 'password': password or os.getenv('AIIDA_BROKER_PASSWORD', BROKER_DEFAULTS['password']), + 'host': host or os.getenv('AIIDA_BROKER_HOST', BROKER_DEFAULTS['host']), + 'port': port or int(os.getenv('AIIDA_BROKER_PORT', BROKER_DEFAULTS['port'])), + 'virtual_host': virtual_host or os.getenv('AIIDA_BROKER_VIRTUAL_HOST', BROKER_DEFAULTS['virtual_host']), + 'heartbeat': heartbeat or int(os.getenv('AIIDA_BROKER_HEARTBEAT', BROKER_DEFAULTS['heartbeat'])), } LOGGER.info(f'Attempting to connect to RabbitMQ with parameters: {connection_params}') diff --git a/src/aiida/cmdline/commands/cmd_profile.py b/src/aiida/cmdline/commands/cmd_profile.py index 55d59f706e..047126a38c 100644 --- a/src/aiida/cmdline/commands/cmd_profile.py +++ b/src/aiida/cmdline/commands/cmd_profile.py @@ -130,6 +130,7 @@ def profile_setup(): @verdi_profile.command('configure-rabbitmq') # type: ignore[arg-type] @arguments.PROFILE(default=defaults.get_default_profile) +@options.FORCE() @setup.SETUP_BROKER_PROTOCOL() @setup.SETUP_BROKER_USERNAME() @setup.SETUP_BROKER_PASSWORD() @@ -138,15 +139,30 @@ def profile_setup(): @setup.SETUP_BROKER_VIRTUAL_HOST() @options.NON_INTERACTIVE(default=True, show_default='--non-interactive') @click.pass_context -def profile_configure_rabbitmq(ctx, profile, **kwargs): +def profile_configure_rabbitmq(ctx, profile, non_interactive, force, **kwargs): """Configure RabbitMQ for a profile. Enable RabbitMQ for a profile that was created without a broker, or reconfigure existing connection details. """ + from aiida.brokers.rabbitmq.defaults import detect_rabbitmq_config + + connection_params = {key.lstrip('broker_'): value for key, value in kwargs.items() if key.startswith('broker_')} + + broker_config = detect_rabbitmq_config(**connection_params) + + if broker_config is None: + echo.echo_warning(f'Unable to connect to RabbitMQ server with configuration: {connection_params}') + if not force: + click.confirm('Do you want to continue with the provided configuration?', abort=True) + else: + echo.echo_success('Connected to RabbitMQ with the provided connection parameters') + profile.set_process_controller(name='core.rabbitmq', config=kwargs) ctx.obj.config.update_profile(profile) ctx.obj.config.store() + echo.echo_success(f'RabbitMQ configuration for `{profile.name}` updated to: {connection_params}') + @verdi_profile.command('list') def profile_list(): diff --git a/tests/cmdline/commands/test_profile.py b/tests/cmdline/commands/test_profile.py index 9718bd06f5..781a8b3cfa 100644 --- a/tests/cmdline/commands/test_profile.py +++ b/tests/cmdline/commands/test_profile.py @@ -284,8 +284,9 @@ def test_configure_rabbitmq(run_cli_command, isolated_config): # Now run the command to configure the broker options = [profile_name, '-n'] - run_cli_command(cmd_profile.profile_configure_rabbitmq, options, use_subprocess=False) + cli_result = run_cli_command(cmd_profile.profile_configure_rabbitmq, options, use_subprocess=False) assert profile.process_control_backend == 'core.rabbitmq' + assert 'Connected to RabbitMQ with the provided connection parameters' in cli_result.stdout # Verify that running in non-interactive mode is the default options = [ @@ -293,9 +294,16 @@ def test_configure_rabbitmq(run_cli_command, isolated_config): ] run_cli_command(cmd_profile.profile_configure_rabbitmq, options, use_subprocess=True) assert profile.process_control_backend == 'core.rabbitmq' + assert 'Connected to RabbitMQ with the provided connection parameters' in cli_result.stdout + + # Verify that configuring with incorrect options and `--force` raises a warning but still configures the broker + options = [profile_name, '-f', '--broker-port', '1234'] + cli_result = run_cli_command(cmd_profile.profile_configure_rabbitmq, options, use_subprocess=False) + assert 'Unable to connect to RabbitMQ server with configuration:' in cli_result.stdout + assert profile.process_control_config['broker_port'] == 1234 # Call it again to check it works to reconfigure existing broker connection parameters - options = [profile_name, '-n', '--broker-host', 'rabbitmq.broker.com'] - run_cli_command(cmd_profile.profile_configure_rabbitmq, options, use_subprocess=False) - assert profile.process_control_backend == 'core.rabbitmq' - assert profile.process_control_config['broker_host'] == 'rabbitmq.broker.com' + options = [profile_name, '-n', '--broker-port', '5672'] + cli_result = run_cli_command(cmd_profile.profile_configure_rabbitmq, options, use_subprocess=False) + assert 'Connected to RabbitMQ with the provided connection parameters' in cli_result.stdout + assert profile.process_control_config['broker_port'] == 5672