Skip to content

verdi setup: improve validation and help string of broker virtual host #4408

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 28, 2020
Merged

verdi setup: improve validation and help string of broker virtual host #4408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions aiida/cmdline/params/options/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,10 +315,10 @@ def decorator(command):

BROKER_VIRTUAL_HOST = OverridableOption(
'--broker-virtual-host',
type=types.HostnameType(),
type=click.types.StringParamType(),
Copy link
Member

@ltalirz ltalirz Sep 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Just to understand: Didn't the HOSTNAME_REGEX already exclude leading slashes?
https://github.com/sphuber/aiida-core/blob/dbbea0e3582894f8ef88e0f534db7560a1d8994d/aiida/cmdline/params/types/strings.py#L62
Why not keep the HostnameType?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to understand: Didn't the HOSTNAME_REGEX already exclude leading slashes?

Yes, which is exactly the problem. A virtual host with one or more slashes is perfectly valid. See also my commit message:

On top of that, slashes would fail the validation outright, even though these are common in virtual hosts.

In the final paragraph of the commit message I explain why we resort to a type with as little restrictions as possible, namely a simple string type. I checked the RabbitMQ docs for virtual host naming rules but couldn't find it, so I asked on the mailing list and experimented: https://groups.google.com/u/1/g/rabbitmq-users/c/ZT56iwGbxy0 The last response is what I found by experimenting, almost anything goes. So I think it is best not to restrict anything in our interface.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, which is exactly the problem. A virtual host with one or more slashes is perfectly valid.

Well, I was going by the help string, in which you ask people not to add the leading slash.

But I see now that these virtual hosts can contain almost any string anywhere, thanks for the info!

default=BROKER_DEFAULTS.virtual_host,
show_default=True,
help='Name of the virtual host for the message broker. Forward slashes need to be encoded'
help='Name of the virtual host for the message broker without leading forward slash.'
)

REPOSITORY_PATH = OverridableOption(
Expand Down
2 changes: 1 addition & 1 deletion docs/source/intro/installation.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,7 @@ The following parameters can be configured:
+--------------+---------------------------+---------------+-------------------------------------------------------------------------------------------------------------------------+
| Port | ``--broker-port`` | ``5672`` | The port to which the server listens. |
+--------------+---------------------------+---------------+-------------------------------------------------------------------------------------------------------------------------+
| Virtual host | ``--broker-virtual-host`` | ``''`` | Optional virtual host. If defined, needs to start with a forward slash. |
| Virtual host | ``--broker-virtual-host`` | ``''`` | Optional virtual host. Should not contain the leading forward slash, this will be added automatically by AiiDA. |
+--------------+---------------------------+---------------+-------------------------------------------------------------------------------------------------------------------------+


Expand Down
8 changes: 4 additions & 4 deletions docs/source/reference/command_line.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,8 +500,8 @@ Below is a list with all available subcommands.

--broker-host HOSTNAME Hostname for the message broker. [default: 127.0.0.1]
--broker-port INTEGER Port for the message broker. [default: 5672]
--broker-virtual-host HOSTNAME Name of the virtual host for the message broker. Forward
slashes need to be encoded [default: ]
--broker-virtual-host TEXT Name of the virtual host for the message broker without
leading forward slash. [default: ]

--repository DIRECTORY Absolute path to the file repository.
--config FILEORURL Load option values from configuration file in yaml
Expand Down Expand Up @@ -638,8 +638,8 @@ Below is a list with all available subcommands.
required]

--broker-port INTEGER Port for the message broker. [default: 5672; required]
--broker-virtual-host HOSTNAME Name of the virtual host for the message broker. Forward
slashes need to be encoded [default: ; required]
--broker-virtual-host TEXT Name of the virtual host for the message broker without
leading forward slash. [default: ; required]

--repository DIRECTORY Absolute path to the file repository.
--config FILEORURL Load option values from configuration file in yaml
Expand Down