-
-
Notifications
You must be signed in to change notification settings - Fork 252
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
Add possibility to include multiple configs #333
Comments
@kbza I moved the conversation to a new ticket. |
So, if you would like to implement it, you would need to change src/model/script_config.py file. First of all, to reduce the work for you, here is the modification in src/model/model_helper.py, which you would need:
After that, in script_config.py:338, replace that line with: After that you would need to adjust |
Hello there, I've managed to implement the solution you proposed (actually it was almost fully done by you). Practically, the only change was to merge the dicts recursively in case of path being a list inside
There was a caveat that I think is related to the way I've defined the script conf and this particular implementation:
When I select more than 1 parameter in the first multiselect ("Server Type"), it tries to load every selected file for each one of the types selected. It works but warns me everytime: This works though... I think this is a very specific solution that only works for this use-case (using multiselects). For me, the final solution would be:
I've kinda made a config like this work:
But:
But why all this trouble?
I think this changes will improve the portability and will simplify configuration once structured (at least for those of us that plan to install this software dozens of times ;) ). |
Hi @kbza, to be honest, I'm not fully understand your proposal :)
Could you give an example, please?
This sounds fine to me. I had an impression, that you only need multiselect support.
If you could share your code and configuration examples (you can prepare just some test files), I can try to debug and check it |
I war refering to something like "${auth.username}" that can be used in scripts, but this one will be something like "${include_path}". If needed, you may configure it as a server configuration property and use it to refer a common path were every include file will be placed (instead of using the hole path for every file).
This is the one thing we've already achieved with the changes you proposed. The problem is, that I mostly don't want to offer the user the choice to select include files.
You can check the canges here: https://github.com/kbza/script-server/tree/multi-include
of course, this will be refactored prior to any pull request... for now this is just a concept. |
Hi @kbza unfortunately I didn't have time so far to check your code. Will try to do it tomorrow late evening |
My bad there, I made wrong implementation for variable substitution. Here is the updated method:
Regarding this concern, I think your idea is fine. We can just fine-tune the implementation. For example, when you search for required_parameters inside TemplateProperty, you can extract common logic to a method and then the corresponding part in the init method would look smth like:
Where One more feature, which you could add, is support for environment variables, there is a method: model.model_helper.resolve_env_vars. If you add a usage for it (for example after you call |
Will be addressed in #423 |
Done, now "included" option accepts an array. In case of a conflict (same options in different included files), the first defined included file takes precedence. |
Hello there!
Is there any way of including more than one external included file for script? thinking of one of the following:
I'm trying to figure out a way of having configuration files that will be the same across installations, some specific to each one and others specific to the servers.
Is there any way of achiving this in the current version? will you consider implementing something like this in the future?
Thanks in advance.
Originally posted by @kbza in #133 (comment)
The text was updated successfully, but these errors were encountered: