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

[config] Merging lists #153

Open
nemesifier opened this issue May 2, 2020 · 9 comments · May be fixed by #288 or openwisp/openwisp-controller#814
Open

[config] Merging lists #153

nemesifier opened this issue May 2, 2020 · 9 comments · May be fixed by #288 or openwisp/openwisp-controller#814
Projects

Comments

@nemesifier
Copy link
Member

nemesifier commented May 2, 2020

The way configuration is merged with templates now has a quirk:
list elements from different templates are merged with the config.

This is needed for list of objects (eg: interfaces, routes), but it's weird and has caused misunderstanding among users when they try to override list options.

I still haven't come up with an optimal solution.

@nemesifier nemesifier added this to To do general in Roadmap via automation May 2, 2020
@nemesifier nemesifier moved this from To do general to To do (Device management) in Roadmap May 2, 2020
@nemesifier nemesifier added the Hacktoberfest Easy issues for attracting Hacktoberfest participants. label Sep 30, 2020
@nemesifier nemesifier added this to Easy pickings in Hacktoberfest 2020 via automation Sep 30, 2020
@nemesifier nemesifier moved this from Easy pickings to Python | Django in Hacktoberfest 2020 Sep 30, 2020
@nemesifier nemesifier removed the Hacktoberfest Easy issues for attracting Hacktoberfest participants. label Oct 2, 2020
@nemesifier nemesifier removed this from Python | Django in Hacktoberfest 2020 Oct 13, 2020
@okraits
Copy link
Member

okraits commented Jun 16, 2023

Let's list the options we have so we can get closer to a solution.

  1. Items of lists with the same name in different templates are merged, resulting in a list containing all items from all templates. This is the current behaviour.
  2. If there are lists with the same name in different templates, the items of the list in the template with the highest priority will be the items in the resulting list, overriding the items in the other templates. This has the disadvantage that for adding items to a list, all common items need to be listed in several templates.
  3. We could add an option/setting to the list in our templates which defines the behaviour of that list if there is a list with the same name in a template with lower priority: add items to that lower priority list or replace items. This makes it possible to choose the behaviour depending on the usecase.

@nemesisdesign What do you think?

@nemesifier
Copy link
Member Author

@okraits option 3. sounds like a good idea, an "advanced option" which allows to decide if the user wants to add to any previous template or override, to be set on a per template basis and it would act only on previous templates, right?

@okraits
Copy link
Member

okraits commented Jun 19, 2023

@okraits option 3. sounds like a good idea, an "advanced option" which allows to decide if the user wants to add to any previous template or override, to be set on a per template basis and it would act only on previous templates, right?

Yes, each template would basically take the result of the previous templates as input and then override that or add to it, based on the selected advanced option.

@okraits
Copy link
Member

okraits commented Sep 14, 2023

@nemesifier So if you're ok with option 3, I will start to work on it, ok?

@nemesifier
Copy link
Member Author

@okraits how do you intend to implement this? Are we going to add an initialization argument to the library?

You mention adding an "option" to the list, can you expand on this?

@okraits
Copy link
Member

okraits commented Sep 14, 2023

@okraits how do you intend to implement this? Are we going to add an initialization argument to the library?

You mention adding an "option" to the list, can you expand on this?

@nemesifier

To be honest, I have to look into the code first to see what's possible. I'll get back to you with a proposal, ok?

@okraits
Copy link
Member

okraits commented Oct 16, 2023

@nemesifier I thought about the whole topic and possible solutions.

Originally I thought about adding a "meta setting" to every list in the json configuration dictionary (which would mean that we would have to store a list in json in a surrounding dictionary to be able to have a "meta setting") to define wether the corresponding list should be merged with or override an existing list.
But I guess that this is too complex, we would need to extend the json schema and possibly modify all the parsers, converters and renderers.

So I guess the more simple solution is to add a field to the AbstractTemplate model to define the behaviour of lists for each template. I can think of the following behaviours:

  • override/replace an existing list
  • insert items at the beginning of an existing list
  • append items to the end of an existing list

Am I correct that we basically need to modify the following functions to implement the behaviours mentioned above:

What do you think?

@okraits
Copy link
Member

okraits commented Oct 23, 2023

@nemesifier Any opinion on my suggestion?

@okraits
Copy link
Member

okraits commented Nov 9, 2023

@nemesifier I didn't receive a response from you, so I implemented this. Please take a look at it and feel free to ask questions or request changes.

P.S. I will first wait for your feedback and fix the tests afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Roadmap
  
To do (Device management)
2 participants