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

Add additional template functions #569

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

stuggi
Copy link
Contributor

@stuggi stuggi commented Oct 11, 2024

Adds templat functions which:

  • to exec an inline template
  • indent a template
  • remove n continuous empty lines from a template

An example would be a template like:

{{define "my-template"}}new template

content

with empty lines

to
remove
{{end}}
Some other template content and add the rendered from my-template
{{$var := execTempl "my-template" . | removeNewLines 1}}
{{$var}}

@stuggi
Copy link
Contributor Author

stuggi commented Oct 11, 2024

an example for using it in nova for the nova.conf is in openstack-k8s-operators/nova-operator#874

exclude: |
(?x)(
^vendor|
^modules/common/util/template_util_test.go
Copy link
Contributor Author

@stuggi stuggi Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to add this because of the test data for TestRemoveNewLines where I test spaces and tabs in empty lines

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels a bit scary. I would the test data to normal strings with \n and \t to avoid this. But that can be done in a follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the spaces from the test data and returned the config.

@SeanMooney
Copy link
Contributor

we coudl do this but i guess we were trying to avoid defienign our own dsl
I'm concurred that if we do this this way we may end up building a complex templaating system
so we should be very intentional about taking this direction if we do.

@stuggi
Copy link
Contributor Author

stuggi commented Oct 11, 2024

we coudl do this but i guess we were trying to avoid defienign our own dsl I'm concurred that if we do this this way we may end up building a complex templaating system so we should be very intentional about taking this direction if we do.

yes we should not introduce a lot of/further complexity. At least right now I am not aware of something else needed. I can remove the indent func as it was not requested. What I like with this approach is that it uses the default way on how you extend the go templating using these funcs.

@SeanMooney
Copy link
Contributor

we coudl do this but i guess we were trying to avoid defienign our own dsl I'm concurred that if we do this this way we may end up building a complex templaating system so we should be very intentional about taking this direction if we do.

yes we should not introduce a lot of/further complexity. At least right now I am not aware of something else needed. I can remove the indent func as it was not requested. What I like with this approach is that it uses the default way on how you extend the go templating using these funcs.

yep that is a plus as it not a net new way of doing this.
and this filter/fucntion style approach should feel famialr to anyone who has used jinja as well.

}

// template function to remove empty lines if there are > n continuous empty lines
func removeNewLines(n int, in string) (out string) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUTC, this allow people to have n number of lines in between.
I think we may not need this in case conf files, but may be for other templates!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will remove n number of continuous empty lines, like you wanted to achieve, only not covering the special use case for the section headers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep and for readabliyt we shoudl be ensuring that we have the correct spaceing for the headers in the template anyway so I'm fine with the proposal to just remove multiple newlines and leave having a new line before section to how we write the template.

@auniyal61
Copy link

I think its better approach, as there are only template level changes at operator side.

right now, the removeNewLines function is not working as we want/expected, I had tested it locally.

current_nova.conf render

removeNewLines(0, str)

removeNewLines(1, str)

removeNewLines(2, str)

removeNewLines(3, str)

(but that may not be the focus of this patch)

@stuggi
Copy link
Contributor Author

stuggi commented Oct 14, 2024

I think its better approach, as there are only template level changes at operator side.

right now, the removeNewLines function is not working as we want/expected, I had tested it locally.

current_nova.conf render

removeNewLines(0, str)

removeNewLines(1, str)

Isnt' this what you wanted? it will remove multiple continuous empty lines and just keep a single?
Not covering the special header case

removeNewLines(2, str)

removeNewLines(3, str)

(but that may not be the focus of this patch)

@auniyal61
Copy link

I think its better approach, as there are only template level changes at operator side.
right now, the removeNewLines function is not working as we want/expected, I had tested it locally.
current_nova.conf render
removeNewLines(0, str)

Isnt' this what you wanted? it will remove multiple continuous empty lines and just keep a single? Not covering the special header case

yes, after adding for section-header with n =0, it should work
we need this.

@stuggi
Copy link
Contributor Author

stuggi commented Oct 14, 2024

I think its better approach, as there are only template level changes at operator side.
right now, the removeNewLines function is not working as we want/expected, I had tested it locally.
current_nova.conf render
removeNewLines(0, str)

Isnt' this what you wanted? it will remove multiple continuous empty lines and just keep a single? Not covering the special header case

yes, after adding for section-header with n =0, it should work we need this.

I have added a form of your cleanup func from #557 and added you as co-author to the commit msg. I was just not sure if you want your full name listed there as it is not shown in your github account. I can change that, just let me know.

@SeanMooney
Copy link
Contributor

reflecting on this over the weekend i think I'm satisfied that this is a better approach to achieve this usecase, i left one comment inline but I'm mostly happy with this as proposed.

@auniyal61
Copy link

lgtm
thanks for including unit tests and bar.conf as well

Copy link
Contributor

@SeanMooney SeanMooney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks ok to proceed with from my perspective

modules/common/util/template_util.go Show resolved Hide resolved
exclude: |
(?x)(
^vendor|
^modules/common/util/template_util_test.go
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels a bit scary. I would the test data to normal strings with \n and \t to avoid this. But that can be done in a follow up.

Adds template functions which:
- to exec an inline template
- indent a template
- remove n continuous empty lines from a template
- specific to openstack ini files, removes all new lines in sections,
  but have them in section separation.

An example would be a template like:
```
{{define "my-template"}}new template

content

with empty lines

to
remove
{{end}}
Some other template content and add the rendered from my-template
{{$var := execTempl "my-template" . | removeNewLines 1}}
{{$var}}
```

Co-authored-by: auniyal61

Signed-off-by: Martin Schuppert <[email protected]>
Copy link
Contributor

@abays abays left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@olliewalsh olliewalsh merged commit e5c35d2 into openstack-k8s-operators:main Oct 14, 2024
2 checks passed
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.

6 participants