-
Notifications
You must be signed in to change notification settings - Fork 992
feat: Jinja2 template engine in workflow templates #4595
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
base: main
Are you sure you want to change the base?
feat: Jinja2 template engine in workflow templates #4595
Conversation
@EnotShow is attempting to deploy a commit to the KeepHQ Team on Vercel. A member of the Team first needs to authorize it. |
@EnotShow lot of unit tests are failing, can you fix that? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests failing
Also a quick question about backwards compatibility - not sure if I understand if this will still support mustache or we're replacing to Jinja completely? Maybe we should support |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hi @talboren, @shahargl! |
keep/iohandler/iohandler.py
Outdated
def validate_template_syntax(self, template: str) -> bool: | ||
""" | ||
Checks whether the template matches the expected template engine. | ||
|
||
Args: | ||
template (str): The template text to be checked. | ||
|
||
Returns: | ||
bool: True if the template matches the expected engine. | ||
|
||
Raises: | ||
RenderException: If the template contains syntax from a different engine. | ||
""" | ||
# Patterns specific to Jinja2 | ||
jinja2_patterns = { | ||
'statement': r'{%[^%}]+%}', # {% if something %} | ||
'comment': r'{#[^#}]+#}', # {# comment #} | ||
'filters': r'\{\{[^}]+\|[^}]+\}\}', # {{ variable|filter }} | ||
'blocks': r'{%\s*(end)?(if|for|block|macro|set)[^%}]*%}', # {% endif %}, {% endfor %}, etc. | ||
} | ||
|
||
# Patterns specific to Mustache | ||
mustache_patterns = { | ||
'section': r'{{#[^}]+}}', # {{#section}} | ||
'inverted': r'{{(\^|\^!)[^}]+}}', # {{^section}} or {{^!section}} | ||
'end_section': r'{{/[^}]+}}', # {{/section}} | ||
'unescaped': r'{{{[^}]+}}}', # {{{ variable }}} | ||
'comment': r'{{![^}]+}}', # {{! comment }} | ||
'partial': r'{{>[^}]+}}', # {{> partial }} | ||
} | ||
|
||
has_jinja2 = False | ||
has_mustache = False | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a better and simpler way to check jinja2 expressions,
e.g.:
from jinja2 import Environment, TemplateSyntaxError
env = Environment()
# valid
In [4]: env.parse("Hello, {{ name }}!")
Out[4]: Template(body=[Output(nodes=[TemplateData(data='Hello, '), Name(name='name', ctx='load'), TemplateData(data='!')])])
# invalid
In [8]: a = """{% for item in items %}
...: {{ item }}
...: {% endif %}"""
In [9]: env.parse(a)
-------------------------------------------------------------------
TemplateSyntaxError Traceback (most recent call last)
Cell In[9], line 1
----> 1 env.parse(a)
File ~/git/keep/.venv/lib/python3.11/site-packages/jinja2/environment.py:616, in Environment.parse(self, source, name, filename)
614 return self._parse(source, name, filename)
615 except TemplateSyntaxError:
--> 616 self.handle_exception(source=source)
File ~/git/keep/.venv/lib/python3.11/site-packages/jinja2/environment.py:942, in Environment.handle_exception(self, source)
937 """Exception handling helper. This is used internally to either raise
938 rewritten exceptions or return a rendered traceback for the template.
939 """
940 from .debug import rewrite_traceback_stack
--> 942 raise rewrite_traceback_stack(source=source)
File <unknown>:3, in template()
TemplateSyntaxError: Encountered unknown tag 'endif'. Jinja was looking for the following tags: 'endfor' or 'else'. The innermost block that needs to be closed is 'for'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s a good approach, but my goal here isn’t to check whether the Jinja2 template is syntactically valid. Instead, I want to detect if the user accidentally used Mustache syntax when they selected Jinja2 as the template engine (or vice versa), and raise an informative error.
So it's more about catching cross-engine mistakes — for example, using {{#section}}...{{/section}} inside a Jinja2 context, or {% if ... %} inside a Mustache one.
That said, I agree this logic could probably be simplified. I’ll take another look and try to clean it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👑
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems almost done, see comments.
and if you can add more tests it will be great <3 just a big change so want to make sure we are ok.
@EnotShow let me know when you want the final review + merging |
…mplates Signed-off-by: Ihor <[email protected]>
@talboren This is ready for review, but I ran into an issue with Jinja2. Jinja2 doesn't allow hyphens in identifier names when accessed as attributes in templates. To work around this, I implemented a function which replaces code like:
with:
This change uses dictionary-style access, which is valid in Jinja2 for keys that contain hyphens. It seems to work, but I not sure if this solution is proper enough. What you think? |
makes absolute sense! I'll try to review it ASAP. |
…mplates Signed-off-by: Ihor <[email protected]>
…mplates Signed-off-by: Ihor <[email protected]>
Closes #4594
📑 Description
This PR added the whole power of Jinja2 templating to the KeepHQ workflow
✅ Checks
✨ Features
🛠 Improvements
During Jinja2 rendering, only missing top-level keys can be detected — full key paths cannot be tracked.
Example: {{ alert.namespase }} will report {{ namespace }} as missing, but due to how Jinja2's Undefined class works, it cannot identify the full path to the missing value.
It’s not possible to use a tracking dictionary for Jinja2 in the same way as Mustache, because Jinja2 internally converts the context to a plain dict during rendering.