Introduce support for jinja2-based templates.

[albu-diku]

Dependent upon #467.

[jonasbardino]

From a first glance it looks sensible and usable enough.
I still would like a short write-up or analysis of what we want / need and some sort of brief evaluation of available template engines including features and performance. Bonus for thoughts and numbers backing the choice of granularity to be the individual output object entries.
We may still very well go with jinja2 but just to make sure it's in fact the best match.

This being a draft I'll refrain from commenting much on the obvious polish missing before it's ripe for actual merging, but from a quick glance you probably want to fix a few typos in var and function names, go over license headers and consider doc strings :)

[albu-diku]

Remaining lint failures are in pre-existing code unchanged by this PR.

[rasmunk]

After gaining an initial impression, I think the first step is to get some make format-python applied to the mentioned changes to ensure proper formatting and afterwards apply a static code analyzer such as flake8/pylint to cleanup any unused imports and non used variables. From running a simple check against the added mig/lib/templates package a number of such issues are highlighted.

I know that we have existing issues in this area in existing files that wont be caused by changes in here but at least the new introductions should not add to this issue:)

[albu-diku]

After gaining an initial impression, I think the first step is to get some make format-python applied to the mentioned changes to ensure proper formatting and afterwards apply a static code analyzer such as flake8/pylint to cleanup any unused imports and non used variables. From running a simple check against the added mig/lib/templates package a number of such issues are highlighted.

I know that we have existing issues in this area in existing files that wont be caused by changes in here but at least the new introductions should not add to this issue:)

I’ll do what I can on this, I’m just very very worried about introducing conflict with the larger branch after some nasty recent experiences given things were reordered etc on next.

I might have to only do it to newly added files and leave existing places untouched to avoid problems.

[rasmunk]

A more smooth approach then could be is to do this on the larger PR and afterwards pull it into the sub-PRs like this one. I imagine that the other PRs would want the same assurance/work applied to them if it has not already been applied. But yes my initial suggestion was to at least apply it to new files/modules as a baseline, and then if it is not too much hassle to also apply it to new changes in the existing files.

[albu-diku]

I have reformatted the added files but will not go further with anything that was pre-existing - both because it will massively inflate the diff and because we will be applying the automated formatting anyway so I don't see any benefit to the churn.

Pylint and flake 8 are already applied by CI so I don't think it's possible there are still issues lurking in the newly added code. As before I am staying away from messing too much with existing files.

[rasmunk]

pylint and flake8 are indeed applied by the CI, but flake8 is set to --exit-zero by default so it will not fail on issues, but instead just list them things such as the unused imports in the new mig/lib/templates
https://github.com/ucphhpc/migrid-sync/actions/runs/24777959961/job/72501069564#step:9:639

or redefinition of unused import in mig/shared/configuration.py
https://github.com/ucphhpc/migrid-sync/actions/runs/24777959961/job/72501069564#step:9:1268

Due to the existing issues it might be easiest to gather any such issue locally against the new parts to ensure that they indeed to not add additional lines to the already long list of issues.

For instance, with the newly introduced lint-python make target, the issues introduced here should able to be gathered via for instance make lint-python LINT_ENFORCE_DIRS=mig/lib/templates or with ruff/flake8 manually.

[rasmunk]

Its not to say that these things are big issues but likely just a bunch of change-lines that can be removed because they appear to not being used or minor issues that don't amount to actual syntax errors that pylint would catch. In fact it was just the initial static analyse step I had time to do before other tasks demanded my attention. Additional review will follow in due time and likely in their own contained issues.

[albu-diku]

I've done the pass over the files as was requested.

[rasmunk]

It should properly be emphasized in the documentation or elsewhere that it is required that any apps that provide templates are required to use this format. Might even turn into [TEMPLATES] configuration option in the future

[albu-diku]

Agree on the documentation side, but I don''t quite follow the mention of the templates section.

[albu-diku]

Would you be able to clarify your comment?

[rasmunk]

The thought about the templates section, was simply that we would at some point expose the expected template lookup format in the MiGServer.conf template section:

For example as

[TEMPLATES]
base_packages = migux
cache_dir = AUTO
templates_lookup_format = "%s.%s.jinja"

Not that we would customise this initially, but it atleast exposes to the users/admins the expected structure that the base_packages would have to abide by.

[albu-diku]

Hmmm. I’d be very hesitant to do that - it’s an implementation detail and things will not work if it is changed.

My sense about what you’re really after here is documenting of things like how to lay things out, the format, etc. I’d rather work towards capturing that stuff.

[rasmunk]

I would say, that in my mind it is more part of the interface specification for introducing templates. That is that for an external package (or internal) that provides templates must abide by this structure to be compatible with how the TemplateStore loads templates. But as hinted at, this is not a ticket item for me that it could be customisable, I would also be just fine if a constant is defined that defines this structure, such as TEMPLATES_LOAD_FORMAT="%s.%s.jinja" or whatever name works best, that we can include in the docs/description of templates and their requirements when they are implemented.

[rasmunk]

It seems like the mig/assets/templates/.gitkeep path/file is a leftover that is not being used?

Can see it was removed and fixed in the latest changes 👍

[albu-diku]

It seems like the mig/assets/templates/.gitkeep path/file is a leftover that is not being used?

Can see it was removed and fixed in the latest changes 👍

Oh sorry, have been trying to make sure there is always a link as things addressed but I just dealt with that one.

Fwiw this dates back to before package based loading was discovered and everything was reworked to use it.

[rasmunk]

It seems like the mig/assets/templates/.gitkeep path/file is a leftover that is not being used?
Can see it was removed and fixed in the latest changes 👍

Oh sorry, have been trying to make sure there is always a link as things addressed but I just dealt with that one.

Fwiw this dates back to before package based loading was discovered and everything was reworked to use it.

No worries :) and now we just have the 3 remaining things and we should be good to merge.

[rasmunk]

A quick question, has it been validated that the generateconfs.py script works with the introduced template options --templates_cache_dir and --templates_base_packages and that they are available when we generate a configuration during the build in docker-migrid? Seems like they are missing at the end of

migrid-sync/mig/install/generateconfs.py

Line 220 in af81b87

'datasafety_text',

in this and the parent PR.

[albu-diku]

A quick question, has it been validated that the generateconfs.py script works with the introduced template options --templates_cache_dir and --templates_base_packages and that they are available when we generate a configuration during the build in docker-migrid? Seems like they are missing at the end of

migrid-sync/mig/install/generateconfs.py

Line 220 in af81b87

'datasafety_text',

in this and the parent PR.

It was, but appears to have been lost likely in one of the more conflict heavy of the many rebases.