-
Notifications
You must be signed in to change notification settings - Fork 91
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
Should we get rid of the instance-parameter in multi instance plugins? #292
Comments
This was something I implemented already in #127 - but there was the opinion that the instance name should be not coupled with the section name. Further it seems to be not a good idea to have two possibilities to reference the plugin in the configuration (the section name and plugin instance name). But still, I prefer my approach to reduce confusion, so I'm a little bit happy to see others have also issues with it :) When implementing such a thing we should decide if we still need two instance names. I would avoid using two plugin names and just use with the section name of the plugin (which is also already used in |
When working at pr/144 I stumbled across this instance name thing - and fell. |
@msinn i think @cstrassburg had an example where the section name would make problems, but currently i dont remember.. hopefully i do the next days |
In #127 steht es drin. Das habe ich damals so implementiert, damit man überhaupt noch ein default Plugin für die Items hat. Diese bisherige schreibweise der Attribute würde dann ganz wegfallen weil ich immer einen Namen habe und viele Plugins Multiinstance fähig sind. |
Ok, Ein möglicher Lösungsansatz könnte sein, die Instance bei MI Plugins im Hintergrund mitzuführen und bei der Initialisierung aus den Abschnittsnamen zu befüllen. Wenn man Plugins hat, von denen nur eine Instanz geladen wird, würde der Instance Name leer bleiben und die klassische Schreibweise bliebe gültig. Erhalten bliebe dabei weiterhin das Thema, dass ein MI Plugin mit mehreren Item Attributen erwartet, dass jedem Attribut der Instancename hinzugefügt werden muss, obwohl es höchst unwahrscheinlich ist, dass ein Item einzelne Attribute der verschiedenen Instanzen eines Plugins ansprechen möchte. Bei diesem Issue geht es mir als erstes darum, dass ein verständliches einfach zu erklärendes Handling entsteht. |
Nein, das fällt nicht weg. Jedenfalls nicht mit den Anpassungen aus #127. Damit gibt es weiterhin das Defaut-Plugin, was in der Konfiguration hinterlegt werden muss. Vielleicht sollten MI Plugins immer den Instanznamen bei den Items gesetzt haben. Falls nicht gibt es eine Fehlermeldung im Log. Wäre aber dann ein breaking change. Alternativ könnte man einfach das erste MI Plugin in der Konfiguration als das Default-Plugin ansehen. Item-Konfigurationen ohne Instanz würden dem dann zugeordnet. Können dort aber auch mit Instanznamen konfiguriert werden. In beiden Fällen könnte man auf das zusätzliche Vielleicht sollten wir die unterschiedlichen Lösungen als Vorschläge in der Beschreibung oben zusammenfassen. |
@ohinckel Würdest Du an der Version eines "Default Plugins" festhalten wollen, wenn kein @instance am Item Attribut angegeben ist? Das würde eigentlich bedeuten, dass alle Attribute eines items die sich auf das Plugin beziehen, ein @instance haben müssten. So sind aber nicht alle Plugins implementiert. Beispiel AVM Plugin:
Hier bewirkt das Wenn ich 2 Instanzen (willy_tel und telekom) habe, würde eine Konfiguration wir
auch keinen Sinn machen. Ich sehe keinen Fall, bei dem ein Item die Attribute unterschiedlicher Instanzen des selben Plugins referenzieren/nutzen will oder soll. Dadurch entsteht die Frage: Wie wird ein Item auf eine Plugin Instanz gebunden?
Bei 3. wäre z.B. ein Syntax wie folgender denkbar:
Ich werde in den kommenden Tagen die bisherigen (und evtl. noch kommende) Vorschläge im 1. Post zusammenfassen. |
das finde ich irgendwie noch verwirrender als überall @instance zu nutzen Ich frage mich auch, wie das mal wird, wenn items eines tages via GUI konfiguriert werden.. vielleicht auch über den use case nachdenken.. |
Das erste Plugin als Default hatte ich nur vorgeschlagen weil es auch Summen gab die sagten "Alle Items anzupassen, nur weil man eine weitere Instanz konfiguriert ist mir zu viel Aufwand". Mir wäre es am liebsten, wenn man die Instanz angeben muss sobald man mehrere Instanzen konfiguriert hat. In anderen Fällen ist die Abgabe optional. Ich halte daher nicht unbedingt am Default-Plugin-Handling fest. Mir ging es nur um "einfachere Migrationswege". Wie das mit den AVM-Plugin harmoniert kann ich nicht sagen. Die Frage ist auch, ob das dort richtig implementiert ist (Zugriff auf Einstellungen ohne Instanzangabe bei den Items). @msinn ich denke ein Item muss nicht nur einer Plugin-Instanz zugeordnet werden können, sondern kann auch mehreren zugelegt werden. Ausreichen dafür ist eine Einstellung für das Plugin, also mit Angabe der Instanz. |
Und genauso ist es doch implementiert (oder war zumindes so) Wo liegt eigentlich das technische Problem hier? Wenn Du nur eine Plugin Instanz verwendest, musst du nirgendwo etwas angeben. Wenn man mehrere Instanzen verwenden möchte muss man sich Gedanken machen was über die Instanzen laufen soll. |
Wenn man den Kommentaren glauben soll, ist das nicht so: man möchte nicht alle Items anpassen müssen, wenn man später eine zusätzliche Plugin-Instanz konfiguriert, sondern nur die für die neue. Für mich ist das kein Argument, nur weil es einem die Arbeit erspart. Dann bräuchte man sich auch keine Gedanken über das Default Plugin machen. Nur meine Meinung, und evtl. erspart das einen Anwender tatsächlich Arbeit. Mit wäre es egal, aber dazu diskutieren wir hier ja.
Ob man das braucht oder unterstützen sollte weiß ich auch nicht so genau. Vielleicht handelt man sich damit nur noch mehr Ärger ein. Das AVM Plugin beispielsweise verwendet aber Item-Konfiguration mit und ohne Instanzangaben (siehe oben). Direktes Verwenden von Item.conf macht's möglich und wird auch bisher nicht verhindert. Ich habe aber auch z.B. das Database Plugin mit mehreren Instanzen konfiguriert (normales Logging und Reporting Logging) und habe somit Items die an mehreren Instanzen eines Plugins gebunden sind. Ich könnte mir auch noch andere Fälle vorstellen (z.B. operationlog Plugin mit dem man Logs parallel in unterschiedlichen Logs verteilen möchte). So ganz abwegig ist das also nicht. |
Ich finde die nicht zwingend einheitliche Benennung des Plugins in der plugin.yaml sowie die Instanz auch nicht hilfreich. Wenn nur ein Plugin da ist, braucht man keine Angaben zur Instanz o.ä. Ich wäre dafür, dass bei mehreren Instanzen eines Plugins dann z.B. den Namen des Plugins aus der plugin.yaml als Instanzbezeichner benutzt. Die Notwendigkeit bzw. Möglichkeit, in einem Item mehrere Instanzen desselben Plugins zu verwenden, sehe ich auch nicht. Es wäre also auch eine "item-weite" Deklaration möglich, wie oben schon genannt
Damit würde komplett auf die zusätzliche Deklaration von Nachteilig wäre sicher, dass es mehr Schreibaufwand ist, wenn nur ein oder zwei Attribute verwendet werden. |
Noch eine Idee: im Prinzip wie oben den "identifier" aus der plugin.yaml
Die Mechaniken, das auszulesen, sind alle vorhanden und nonbreaking; allein der Wechsel des Instanzbezeichners
Das sollte sogar relativ simpel per Skript machbar sein. Wenn wir entscheiden, diesen Weg zu gehen, schreib' ich das Skript ;) (bash oder Logik? :-D ) |
Letzteren Ansatz mit dem Attribut finde ich nicht so schlecht - das korreliert auch mit der Funktion, wie sie momentan bei structs umgesetzt ist. |
When configuring more than one instance of a plugin, the instance parameter is used to bind item attributes to a specific instance of a plugin.
On the other hand, we already have an identifier, that points to the specific instance of those plugins. Each configured plugin has a section name in etc/plugin.yaml. That section name could be used for the same purpose.
That way we could solve some irritations about the configuration of multi instance plugins.
If it is the way to go, I would dig into the code to see how to implement it and to show a migration path.
Questions to address
Solutions
TDB.
The text was updated successfully, but these errors were encountered: