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

Remove indentfirst in system-probe.yaml.j2 #361

Merged
merged 3 commits into from
May 20, 2021

Conversation

tasktop-teho
Copy link
Contributor

@tasktop-teho tasktop-teho commented May 17, 2021

What does this PR do?

Removes specification of the obsolete indent filter argument identfirst.
Fixes #360

Motivation

The indentfirst option no longer supported in jinja 3

@tasktop-teho tasktop-teho requested a review from a team as a code owner May 17, 2021 22:49
@bkabrda
Copy link
Contributor

bkabrda commented May 18, 2021

Thanks for submitting this PR! The problem is that using first would break older installations that have Jinja2 < 2.10. I think we could work around this by not using first at all and adding an extra empty line that would be considered the first unindented line (which would make all of the actual lines indented):

system_probe_config:
{% filter indent(width=2) %}

{{ system_probe_config | to_nice_yaml }}
{% endfilter %}

This would result in the first line being blank and unindented, and all of the lines of the system_probe_config would be indented by 2 spaces correctly. The unindented blank line doesn't break yaml parsing, so everything should, in theory, work ok. I'll need to test this approach carefully to ensure it works as expected.

* This will make it compatible with Jinja2 <2.10, and Jinja2 >=3
@tasktop-teho tasktop-teho changed the title Use first instead of indentfirst in system-probe.yaml.j2 Remove indentfirst in system-probe.yaml.j2 May 18, 2021
@tasktop-teho
Copy link
Contributor Author

tasktop-teho commented May 18, 2021

@bkabrda That's a nice approach. What about not specifying the keyword first or indentfirst, and let positional argument takes care of it?
{% filter indent(width=2, True) %}

@bkabrda
Copy link
Contributor

bkabrda commented May 19, 2021

@tasktop-teho you're right that this also possible (and probably even better), but for it to work you'd need to modify the call to be indent(2, True), because you can't use keyword argument before a non-keyword argument. Doing so would lead to an error like this: jinja2.exceptions.TemplateSyntaxError: invalid syntax for function call expression. It would also be awesome if you could include a comment above these calls that would explain that we have to keep using non-keyword args because of backwards compatibility. Thanks!

* Also added comment explaining the use of non-keyword arguments
@bkabrda
Copy link
Contributor

bkabrda commented May 20, 2021

Looks good now, merging. Thank you for the contribution!

@bkabrda bkabrda merged commit 4d84935 into DataDog:master May 20, 2021
@tasktop-teho
Copy link
Contributor Author

Thank you for the constructive advice

@hoshsadiq
Copy link

@bkabrda any ideas when there will be a new release with this change? We're running into the same issue.

@basil
Copy link

basil commented May 24, 2021

Would it be possible to cut a release with this change?

@bkabrda
Copy link
Contributor

bkabrda commented May 25, 2021

Sure, I'll do the pre-release tests today and cut a new release if everything checks out - hopefully today/tomorrow, if I find no issues.

@bkabrda
Copy link
Contributor

bkabrda commented May 25, 2021

I've just released 4.10.0 version of the Datadog role. See https://github.com/DataDog/ansible-datadog/releases/tag/4.10.0 for changelog. It's now also available on Ansible Galaxy: https://galaxy.ansible.com/DataDog/datadog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TASK [Datadog.datadog : Create system-probe configuration file] Failure
4 participants