Add an arbitrary separator for plugin keys

[kindermoumoute]

Fixes #1198

Summary of changes:

• Created an arbitrary separator as constant in core.Separator.
• Every time : was used as a separator for the plugin keys, the character has been changer by core.Separator.

Testing done:

• Build Snap.
• Load a plugin with a : character in the name.
• Load a plugin with a normal name.
• Display the catalog snapctl plugin list.
• Create a task with those 2 plugins.
• Stop this task.
• Unload the normal name plugin.
• Unload the special name plugin with snapctl plugin unload -t <type> -n <name> -v <version>.

@intelsdi-x/snap-maintainers

This separator is arbitrary enough to be unused in a plugin name. 🐢

[IzabellaRaulin]

@kindermoumoute, could you give some example of plugin that has a : character in the name?

I doubt if this change is really needed and if we really want to accept : in plugin name, because that causes that user cannot use shorter way (see point b) to unload the special name plugin , only the way presented in point a is possible:
a) snapctl plugin unload -t <type> -n <name> -v <version>
b) snapctl plugin unload <type>:<name>:<version>

This means inconsistency among others in snapctl command helper:

$ snapctl plugin unload --help

NAME:
   snapctl plugin unload - unload <plugin_type>:<plugin_name>:<plugin_version> or unload -t <plugin_type> -n <plugin_name> -v <plugin_version>

As a user I would like to follow this given instruction and both suggested ways to unload plugin should work fine.

@kindermoumoute, what is your perspective? Is there any plugin which really needs to contain ":" in its name?

[kindermoumoute]

• It's more consistent to point to a unique separator than hard coding it.
• Currently, there is no rules for plugin name. This implicitly means we accept every names, except for uppercase characters that will display a warning. If we keep things like that, it is coherent to accept any character in plugin name, even :.

But your question is legit, do we want : in a plugin name? If no, then we should define rules for that.

[jcooklin]

LGTM

But your question is legit, do we want : in a plugin name? If no, then we should define rules for that.

If snapctl works in the future the way it does today we will need a rule for it (validating that plugin names don't inlude :).

@kindermoumoute, @IzabellaRaulin, all: How would you feel about the proposal that snapctl takes positional arguments for the type, name and version instead of the formatted string "::". This simplification would also allow the removal of the switches --plugin-type -t, --plugin-name -n, --plugin-version -v on the plugin unload command.

[kindermoumoute]

It would make sense, and we could still use advanced format like:

$ snapctl plugin unload publisher file 3
$ snapctl plugin unload processor "my processor" 1

(supposing we want to allow spaces in plugin name)

[nanliu]

I'm curious how does this behave for the rest API?

[IRCody]

I believe the API takes in separate named args for type/name/version and the separator isn't involved until you get into control.

[nanliu]

I meant what happens when we try the following if we allow spaces or slashes in plugin name like 'my processor' or 'my/processor', I think we will just have to URL encode them:

$ curl localhost:8181/v1/plugins/processor/my%20processor
$ curl localhost:8181/v1/plugins/processor/my%2Fprocessor

[jcooklin]

It would require that we url decode the name param (QueryUnescape) in the REST server. I tested adding the ability to handle spaces 👉 jcooklin@da40fd0. I'll open an issue and PR against it tomorrow.

[candysmurf]

It's good to have the flexibility on the namespace separator. I have a couple of questions:

1. As Snap framework includes plugins, will this require plugins to use "core.Separator" from Snap or plugins have the flexibility to use any separator?
2. How much value added by allowing spaces in the plugin names?

[IzabellaRaulin]

@kindermoumoute, @jcooklin, all - I really like the idea that core.Separator is defined and used instead of hardcoded :.

@kindermoumoute, I asked before:

@kindermoumoute, could you give some example of plugin that has a : character in the name?

So, it seems that there is no really use case that some plugin really needs to be named in that way.

Proposal

Based on that, my suggestion is:

• define the core.Separator as ":", so there is no impact how it works before
• define a rule that plugin name mustn't contain core.Separator -> notice, that this rule will be always valid and should be introduced even that
• (plus next step, based on what @candysmurf pointed) consider adding a rule that a space is not allowed in plugin's name depending on the following aspects:
  • is there any benefits gained from allowing spaces in the plugins' names?
  • based on that plugin-maintainers copy the approach used in mock publisher and named their in way mock-file instead mock file, whether it justifies that space in plugin's name is no needed?

Reason

• avoid making a changes in the rest of code before there is no really use cases (I mean that none of existing plugins do not contain : in name)

• if the need of using : will arise in the future, the another changes can be made after some discussion

  So, @jcooklin, in response to:

  How would you feel about the proposal that snapctl takes positional arguments for the type, name and version instead of the formatted string "::"

  I prefer do not change snapctl plugin unload based on the reasons that I put above.

-----------------------------------------------------------------------------------------------------------
@jcooklin, @kindermoumoute, @IRCody, @candysmurf, all - is it reasonable what I proposed? Please, treat it only as suggestion and feel free to comment or even criticize that.

[jcooklin]

Having the separator as simple as : is restrictive and if we are going to do something about it now is our opportunity (before 1.0). The only place that requires that we restrict the use of colons is in snapctl plugin unload however as mentioned above this command is an outlier in terms of how it works. IMO it would be more friendly if "type", "name" and "version" were applied as positional arguments rather than in a special string separated by colons or using flags.

As Snap framework includes plugins, will this require plugins to use "core.Separator" from Snap or plugins have the flexibility to use any separator?

I don't see an obvious case where plugin authors would need to worry themselves with core.Separator unless it's a common character that could reasonably appear in the name of the plugin for the reasons mentioned above.

How much value added by allowing spaces in the plugin names?

I would argue from the other side. Why not allow spaces? Allowing it is trivial.

Most of this comes down to balancing being flexible (user and plugin author friendly) while putting up reasonable restrictions and rules. However this shakes out we need a separate issue and PR that will validate plugin names.

I'm suggesting

• We choose a non-common character for the framework to use as a separator
• Followup with related Issues/PRs to
  • Validate plugin names on load (Validate plugin name on load #1206)
  • Simplify snapctl plugin unload to use positional arguments instead of formatted string and/or flags (Simplify snapctl plugin unload to use positional arguments #1207)
  • Allow spaces in a plugins name
    • URL decode the name in the REST server see jcooklin@da40fd0

[candysmurf]

I can see the benefit of this PR set the key separator as a variable in one place instead of many files so that it can change quickly in the future.

If no user requested to have spaces for a plugin name, I vote not to deal with it for Snap 1.0.

Positioning probably is the better solution as @jcooklin said. We use positions now anyway.

[jcooklin]

LGTM