Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI: Add RabbitMQ options to verdi profile setup #6453

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jun 5, 2024

The verdi profile setup command was added to replace the deprecated command verdi setup. The new command dynamically generates a subcommand for each installed storage plugin.

However, the new command did not allow to configure the connection parameters for the RabbitMQ broker, unlike verdi setup. These common options are now added to the subcommands.

In addition, a new option is added --use-rabbitmq/--no-use-rabbitmq. This flag is on by default, to keep the old behavior of verdi setup. When toggled to --no-use-rabbitmq, the RabbitMQ configuration options are no longer required and are also not prompted for. The profile is then configured without a broker.

@sphuber sphuber requested a review from mbercx June 5, 2024 15:06
@sphuber sphuber force-pushed the fix/verdi-profile-setup-add-rabbitmq-options branch from e6ffbad to 9665a16 Compare June 5, 2024 15:45
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.74%. Comparing base (ef60b66) to head (ad7bef6).
Report is 112 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6453      +/-   ##
==========================================
+ Coverage   77.51%   77.74%   +0.24%     
==========================================
  Files         560      561       +1     
  Lines       41444    41752     +308     
==========================================
+ Hits        32120    32456     +336     
+ Misses       9324     9296      -28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sphuber sphuber force-pushed the fix/verdi-profile-setup-add-rabbitmq-options branch from 9665a16 to 19cbf11 Compare June 6, 2024 10:51
Copy link
Contributor

@GeigerJ2 GeigerJ2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just took it for a spin and looks all good to me. Only noted a small typo.

src/aiida/storage/sqlite_dos/backend.py Outdated Show resolved Hide resolved
sphuber and others added 2 commits June 7, 2024 12:32
The `verdi profile setup` command was added to replace the deprecated
command `verdi setup`. The new command dynamically generates a
subcommand for each installed storage plugin.

However, the new command did not allow to configure the connection
parameters for the RabbitMQ broker, unlike `verdi setup`. These common
options are now added to the subcommands.

In addition, a new option is added `--use-rabbitmq/--no-use-rabbitmq`.
This flag is on by default, to keep the old behavior of `verdi setup`.
When toggled to `--no-use-rabbitmq`, the RabbitMQ configuration options
are no longer required and are also not prompted for. The profile is
then configured without a broker.
@sphuber sphuber force-pushed the fix/verdi-profile-setup-add-rabbitmq-options branch from bb7b210 to ad7bef6 Compare June 7, 2024 10:32
@sphuber
Copy link
Contributor Author

sphuber commented Jun 7, 2024

Thanks for the review @GeigerJ2

@sphuber sphuber merged commit f553f80 into aiidateam:main Jun 7, 2024
9 checks passed
@sphuber sphuber deleted the fix/verdi-profile-setup-add-rabbitmq-options branch June 7, 2024 11:00
@mbercx
Copy link
Member

mbercx commented Jun 8, 2024

Hi guys, again sorry for being a late party-pooper. My main question is if we want to add these RabbitMQ-specific options to all of the setup commands. Now that we have a command for reconfiguring rabbitmq, can't we just use the defaults and then point the user to this command in case they want to use others?

I think we already foresee that we want to try and add a different broker than RabbitMQ. Presumably the options for this broker might be different. Are we going to add all the options for that broker here as well? Should we then also rename the rabbitmq options? (which are now all --broker-X).

I would stick with just adding --use-rabbitmq / --no-use-rabbitmq, use the defaults for the configuration, add a check to see if it worked and point them to the configure-rabbitmq command if it failed.

mikibonacci pushed a commit to mikibonacci/aiida-core that referenced this pull request Sep 3, 2024
The `verdi profile setup` command was added to replace the deprecated
command `verdi setup`. The new command dynamically generates a
subcommand for each installed storage plugin.

However, the new command did not allow to configure the connection
parameters for the RabbitMQ broker, unlike `verdi setup`. These common
options are now added to the subcommands.

In addition, a new option is added `--use-rabbitmq/--no-use-rabbitmq`.
This flag is on by default, to keep the old behavior of `verdi setup`.
When toggled to `--no-use-rabbitmq`, the RabbitMQ configuration options
are no longer required and are also not prompted for. The profile is
then configured without a broker.
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.

3 participants