-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Validate plugin compatibility with version ranges #642
Conversation
In plugin |
Lets start with some automated test examples that'd demonstrate how it works and what kind of edge cases there may be. |
Should we continue with this? |
@hairyhum I haven't looked at it properly yet, sorry. |
Maybe it makes sense to include version checks for plugins depending on plugins. |
ec9c3ff
to
03a7fde
Compare
Here are possible error messages: Rabbitmq version mismatch:
Rabbitmq version is not specified:
Plugin dependency version do not match: (specified required version for
Plugin will not be loaded in case of version mismatch. But will be loaded if required version is not specified. There should be some way to inform about it. @michaelklishin @dumbbell @bdshroyer any ideas on improving messages? |
"Problem loading a plugin…" I also don't think that using a pair of two versions will be very clear to the user. |
Error message format example:
When running |
@@ -1,7 +1,7 @@ | |||
{application, rabbit, %% -*- erlang -*- | |||
[{description, "RabbitMQ"}, | |||
{id, "RabbitMQ"}, | |||
{vsn, "0.0.0"}, | |||
{vsn, "3.7.0"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value is updated at build time. @dumbbell do you agree with this default value bump?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, commited accidentally.
@@ -245,19 +253,103 @@ prepare_plugins(Enabled) -> | |||
AllPlugins = list(PluginsDistDir), | |||
Wanted = dependencies(false, Enabled, AllPlugins), | |||
WantedPlugins = lookup_plugins(Wanted, AllPlugins), | |||
|
|||
{ValidPlugins, Problems} = validate_plugins(WantedPlugins), | |||
%TODO: error message formatting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we address this?
@hairyhum this is on the finish line but there are still two things to do:
Specifically, with the following added to
and attempt to start the broker with `make run-broker PLUGINS='rabbitmq_management',
|
Scratch the above, it's due to the fact that in development plugin version was an empty string. When I set it to a realistic value, I get what I expect:
So yeah, this seems to be mostly about copy review and a bit more testing, e.g. with a release build. |
I guess we can skip validations if plugin version is empty, assuming it's development version. Or just warn users that there are empty versions, which won't be checked. |
@hairyhum OK, that sounds like a good idea. Since this is in master, we should also add debug logging for what plugins were checked against what version ranges. |
case is_version_supported(Version, Versions) of | ||
true -> Acc; | ||
false -> | ||
[{{version_mismatch, Version, Versions}, Name} | Acc] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should rename this to dependency_version_mismatch
to be more specific.
Proposal for #591
In plugin
.app.src
rabbitmq versions will be defined like{rabbitmq_versions, ["3.5.4", "3.6.0"]}
, which means that this plugin valid for versions "3.5.4" and higher "3.5.x" and all "3.6.x" versions.The same format is used for plugin dependencies e.g:
Requires
rabbitmq_management
dependency version to be3.7.2
or higher for3.7.x
series andrabbitmq_federation
version to be any of3.7.x
series.