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: Remove the RabbitMQ options from verdi profile setup #6480

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Jun 19, 2024

For the vast majority of use cases, users will have a default setup for RabbitMQ and so the default configuration will be adequate and so they will not need the options in the command. On the flipside, showing the options by default can makes the command harder to use as users will take pause to think what value to pass.

Since there is the verdi profile configure-rabbitmq command now that allows to configure or reconfigure the RabbitMQ connection parameters for an existing profile, it is fine to remove these options from the profile setup. Advanced users that need to customize the connection parameters can resort to that separate command.

@sphuber sphuber force-pushed the fix/verdi-profile-setup-remove-rmq-options branch from 19be86c to a55bc60 Compare June 19, 2024 15:59
@sphuber sphuber requested review from agoscinski and mbercx June 19, 2024 15:59
Copy link
Contributor

@agoscinski agoscinski left a comment

Choose a reason for hiding this comment

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

Logic for setting up rabbitmq when user sets it to False seems a bit weird to me.

@sphuber sphuber force-pushed the fix/verdi-profile-setup-remove-rmq-options branch from a55bc60 to e22eb76 Compare June 20, 2024 09:18
@sphuber
Copy link
Contributor Author

sphuber commented Jun 20, 2024

Logic for setting up rabbitmq when user sets it to False seems a bit weird to me.

Thanks @agoscinski . I misunderstood your comment and thought you wanted to have the hint for verdi profile configure-rabbitmq to be displayed always. I actually think this makes sense, but also agree that the current wording seems misleading because if the user specifies --no-use-rabbitmq it seems like the command still tried it but failed.

Anyway I have refactored it in the vein of your suggestion being extra clear of the three different pathways. All branches still reference the configure-rabbitmq command, but in the case of --no-use-rabbitmq the command clearly states first that the profile is not configuring RMQ as a broker. I think it should be clear enough now.

Copy link

codecov bot commented Jun 20, 2024

Codecov Report

Attention: Patch coverage is 92.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 77.76%. Comparing base (ef60b66) to head (835053e).
Report is 114 commits behind head on main.

Files with missing lines Patch % Lines
src/aiida/cmdline/commands/cmd_profile.py 90.91% 2 Missing ⚠️
src/aiida/cmdline/commands/cmd_presto.py 94.12% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6480      +/-   ##
==========================================
+ Coverage   77.51%   77.76%   +0.26%     
==========================================
  Files         560      561       +1     
  Lines       41444    41795     +351     
==========================================
+ Hits        32120    32499     +379     
+ Misses       9324     9296      -28     

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

@sphuber sphuber requested a review from agoscinski June 20, 2024 11:15
broker_config = detect_rabbitmq_config()

if broker_config is None:
echo.echo_warning(f'RabbitMQ server not reachable with default connection parameters: {broker_config}.')
Copy link
Member

Choose a reason for hiding this comment

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

Note that I was quickly testing this in the field, and got the following

Warning: RabbitMQ server not reachable with default connection parameters: None.

Since if the connection fails, the detect_rabbitmq_config() command returns None. Maybe it should actually return a tuple of the full config and if the connection was successful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, very sloppy on my part, apologies. I blame trying to fix these things while on holiday. I am not a fan of communicating success/error through return values, so I refactored to use exceptions instead.

@sphuber sphuber force-pushed the fix/verdi-profile-setup-remove-rmq-options branch 3 times, most recently from 4d89e48 to 2678213 Compare June 24, 2024 14:13
For the vast majority of use cases, users will have a default setup for
RabbitMQ and so the default configuration will be adequate and so they
will not need the options in the command. On the flipside, showing the
options by default can makes the command harder to use as users will
take pause to think what value to pass.

Since there is the `verdi profile configure-rabbitmq` command now that
allows to configure or reconfigure the RabbitMQ connection parameters
for an existing profile, it is fine to remove these options from the
profile setup. Advanced users that need to customize the connection
parameters can resort to that separate command.
@sphuber sphuber force-pushed the fix/verdi-profile-setup-remove-rmq-options branch from 2678213 to 835053e Compare June 24, 2024 15:28
Copy link
Contributor

@agoscinski agoscinski left a comment

Choose a reason for hiding this comment

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

Thanks for the work, looks fine for me now!

You understood me correctly the first time, it was just a shallow review and I did not look at the logic carefully till the second review. I like the current solution.

@GeigerJ2 GeigerJ2 added the priority/critical-blocking must be resolved before next release label Jun 25, 2024
@mbercx mbercx self-requested a review June 28, 2024 08:16
Copy link
Member

@mbercx mbercx left a comment

Choose a reason for hiding this comment

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

Did some more field testing, LGTM! 🚀 Thanks again @sphuber

@sphuber sphuber merged commit 6316099 into aiidateam:main Jun 28, 2024
11 checks passed
@sphuber sphuber deleted the fix/verdi-profile-setup-remove-rmq-options branch June 28, 2024 08:24
mikibonacci pushed a commit to mikibonacci/aiida-core that referenced this pull request Sep 3, 2024
…m#6480)

For the vast majority of use cases, users will have a default setup for
RabbitMQ and so the default configuration will be adequate and so they
will not need the options in the command. On the flipside, showing the
options by default can makes the command harder to use as users will
take pause to think what value to pass.

Since there is the `verdi profile configure-rabbitmq` command now that
allows to configure or reconfigure the RabbitMQ connection parameters
for an existing profile, it is fine to remove these options from the
profile setup. Advanced users that need to customize the connection
parameters can resort to that separate command.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/critical-blocking must be resolved before next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants