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

Get all variants of a plugin with different contract #1006

Closed
markus2330 opened this issue Oct 3, 2016 · 15 comments
Closed

Get all variants of a plugin with different contract #1006

markus2330 opened this issue Oct 3, 2016 · 15 comments
Labels

Comments

@markus2330
Copy link
Contributor

Thanks to @petermax2 in #559 Elektra plugins got a way to validate their configuration.
What is missing is a way to query/specify every possible configuration of plugin's configuration as needed for #994. For example if you specify lenses for augeas (see #977) augeas supports different configuration file formats.

The PluginDatabase (src/libs/tools/include/plugindatabase.hpp) should be able to list every possible plugin configuration (with different contract). To not hardcode the list of plugins+configs, we need a way to query plugins to tell us about it.

Different proposals:

Specification

Allow to specify plugin configuration with Elektra's types (e.g. enum), then mark configuration parts that will change the contract (e.g. use different configuration file formats when the config is changed).

For example, augeas could specify:

[spec/lens]
   check/enum = 'access', 'activemq_conf', 'activemq_xml',...
   changes = contract

(the list after check/enum would be a directory listing of /usr/share/augeas/lenses/dist)

Query

Add a function similar to checkconf (#559), for example genconf, that returns every configuration where the plugin's contract would change.

E.g. augeas's genconf would simply return a keyset based on /usr/share/augeas/lenses/dist:

#0/lens = access
#1/lens = active_mq

where #0, #1, .. is an array with every possible configuration with different contracts.

@Namoshek
Copy link
Contributor

Namoshek commented Oct 5, 2016

Would it be possible to have multiple configurable variables (e.g. lens and relpath to specify if exports should contain only relative paths)? And what would genconf return in such cases where we have multiple variables (maybe dangerous with many variables if it generates a * b * c * ... options)?

@markus2330
Copy link
Contributor Author

Yes, it is possible, but none of the current plugins has this issue. If augeas adds some paths it would not matter, because paths do not influence the contract. (Or what do you mean with relpath?)

The proposal is basically about extensional vs. intensional. And declarativ vs. imperativ.

If we already had a language to describe non-trivial configuration problems and a full toolset to do such things as yielding possible configurations, we clearly would use the specification, unfortunately it is still a work-in-progress (i.e. a moving target).

@Namoshek
Copy link
Contributor

Namoshek commented Oct 5, 2016

If augeas adds some paths it would not matter, because paths do not influence the contract. (Or what do you mean with relpath?)

The example was meant to be a boolean, sorry if it wasn't clear enough. So if relpath is set, no absolute kdb paths would be exported (something like that should be a standard for all plugins 😄).

But my concern was more about what happens when there are too many options generated, e.g. three variables à eight options would already yield 512 options, or not? Only because we currently don't have such plugins doesn't mean we shouldn't consider it. I think there are many useful configuration options that could be implemented.

@markus2330
Copy link
Contributor Author

Augeas only supports relative paths. Plugins which currently use absolute paths, are going to be changed (see #51).

But my concern was more about what happens when there are too many options generated, e.g. three variables à eight options would already yield 512 options, or not? Only because we currently don't have such plugins doesn't mean we shouldn't consider it. I think there are many useful configuration options that could be implemented.

Yes, the concern is valid and for the long term a specification is more powerful and generic. The main consideration of genconf are:

  • it is less effort (no specification language + tool (that generate all possibilites based on that specification) are required).
  • it better fits to checkconf (which, in theory, could also be replaced by a specification)

But let us discuss it tomorrow. My idea is to reduce effort on issues of the configuration problem the plugins have and focus on configuration problems applications have.

@Namoshek
Copy link
Contributor

Namoshek commented Oct 5, 2016

I totally agree with you! The simpler the solution, the better in the end. I also doubt we will get huge problems in near future, but it may be a problem to change such a system later on if it gets necessary. So I just wanted to throw my concerns in. For my purpose, genconf is absolutely fine!

markus2330 pushed a commit that referenced this issue Oct 7, 2016
@markus2330
Copy link
Contributor Author

Does the decision cover everything needed?

@Namoshek
Copy link
Contributor

Namoshek commented Oct 7, 2016

I think your proposal looks very promising and covers everything we discussed so far. But wouldn't it be better to use an array instead of named keys here? E.g. instead of

access
access/config
access/config/lens = Access.lns
access/infos
access/infos/format = access
aliases
aliases/config
...

use

#0
#0/config
#0/config/lens = Access.lns
#0/infos
#0/infos/format = access
#1
#1/config
...

Or did you have some special use case in mind for it?

@markus2330
Copy link
Contributor Author

markus2330 commented Oct 7, 2016

Yes, with arrays the overwrite feature (user defined plugin variants) would not work well, thus the name. I clarified that a bit in 8348e4c. (There are still corner cases open for implementation).

The decision does not prohibit arrays though, so if the array entries are stable (e.g. not dependent on file system content) they might be useful.

@Namoshek
Copy link
Contributor

Because it is at the moment the last open task for the rest-backend: Am I supposed to implement the decision or are you going to do it?

By the way: after reading the decision again, I noticed that the proposed function int genconf (KeySet * ks, Key * errorKey) has a different argument order than int elektraPluginCheckConfig (Key * errorKey, KeySet * conf), which in return is already inconsistent with int elektraPluginOpen (Plugin * handle, Key * errorKey). Because you referred to checkconf explicitely in the decision, we should probably switch the argument order?

@markus2330
Copy link
Contributor Author

Sorry that I did not have time to answer, I am still at a conference.

Could you fix this issue please?

Yes, the argument order should be like in genconfig.

@Namoshek
Copy link
Contributor

Namoshek commented Nov 1, 2016

So you mean changing existing usages of checkconfig? Doesn't that (possibly) break other plugins (developed by users outside the repo)?

@markus2330
Copy link
Contributor Author

Thanks for this consideration! But as checkconf was not even mentioned by name in the release notes we can think of it as internal.

@petermax2 Did you have a specific reason for this order of arguments?

It sounds more convincing to me to have the relevant data as first argument and the error information as second argument.

Another signature I consider is:
int genconf/checkconf (Plugin * handle, KeySet * ks, Key * errorKey)
so that plugins get a chance to do some initialization (via open) before validating/generating the config. It might be necessary for python/lua plugins. But this somehow also raises the question why we did not do the validation in open (and not having checkconf at all. I do not really propose that we should do it this way, but we should have a convincing answer to this question, e.g. that we need to call checkconf many times.).

We should definitely write a decision about genconf/checkconf, there are quite a view design considerations involved which we seemingly already have forgotten.

@Namoshek
Copy link
Contributor

Namoshek commented Nov 1, 2016

But as checkconf was not even mentioned by name in the release notes we can think of it as internal.

Ok, didn't notice that. In this case I think a change is fine.

It sounds more convincing to me to have the relevant data as first argument and the error information as second argument.

Absolutely. Error information is also output, whereas I would always put input arguments first in the list.

so that plugins get a chance to do some initialization (via open) before validating/generating the config. It might be necessary for python/lua plugins.

What exactly do you have in mind here?

Btw. I agree with you that checking the configuration in open would probably make more sense, but in the end every maintainer would create an own function for checkconfig (at least internally), so why not make it an explicit function of the contract by standard? Maybe it is useful in some situations. And changing open could also lead to problems, because for checkconf we have one more return value ("config broken, but it was fixed so that it is fine now"). If we omit this info, we somehow introduce undefined behavior (or not?).

@petermax2
Copy link
Member

Did you have a specific reason for this order of arguments?

No, we may change it as you see fit.

@markus2330
Copy link
Contributor Author

What exactly do you have in mind here?

I had in mind that the PluginDatabase keeps all plugins in an open state (currently it reopens them for every query, which is really expensive) and then the plugin handle can easily passed as first argument. Then plugins like the python plugin can initialize the python interpreter in open and use it during genconfig and checkconfig.

If you see a way to make this improvement for the PluginDatabase, we should also pass the handle in all functions.

No, we may change it as you see fit.

Then we certainly should swap the parameters, even if we do not introduce the plugin handle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants