Fixes extra newline from confs

[auniyal61]

As we are using templates to dynamicaly create confs on conditions, Often extra newlines are getting added in confs, we should remove them.

[stuggi]

This seems to be not correct for e.g. this case https://go.dev/play/p/5ms4rPxaX0l where you have 2 new lines:

line 1
line 2


line 3

it would result in

line 1
line 2line 3

Can you explain what you try to achieve? Do you want to replace any empty line from the resulting rendered template?

FYI, a commit to fix the govet was just merged. you'd have to rebase.

[gibizer]

@stuggi How would you feel about adding a post processing hook (a function parameter) to the untils.Template so the service operators can implement any kind of post processing after the template is rendered? Would it be too much complexity?

[stuggi]

could you explain the motivation of doing it as a separate task, instead of updating the template to suppress new lines https://pkg.go.dev/text/template#hdr-Text_and_spaces / https://docs.gomplate.ca/syntax/#suppressing-leading-newlines ?

[auniyal61]

it would result in

line 1
line 2line 3

so initially when I tested it with nova-operator, I decided to go with below because we need atleast 2 lines for new section.
s = strings.ReplaceAll(s, "\n\n\n", "\n")
missed '\n' here, as I tested with sample conf.file adding at https://github.com/openstack-k8s-operators/lib-common/tree/main/modules/common/util/testdata/templates/testservice/config

could you explain the motivation of doing it as a separate task, instead of updating the template to suppress new lines https://pkg.go.dev/text/template#hdr-Text_and_spaces / https://docs.gomplate.ca/syntax/#suppressing-leading-newlines ?

we tried that here openstack-k8s-operators/nova-operator#842

but it seems confusing, also conf did not rendered properly.
so added here, so this can reflect on all conf files.

[SeanMooney]

could you explain the motivation of doing it as a separate task, instead of updating the template to suppress new lines https://pkg.go.dev/text/template#hdr-Text_and_spaces / https://docs.gomplate.ca/syntax/#suppressing-leading-newlines ?

yes so we have looked at doing that twice
amit ahs a draft and it is very messy and makes the template harder to read.

openstack-k8s-operators/nova-operator#855

I did a POC of that about 18 months ago when we were refactoring our templates
and abandoned it for the same reason

the current apporch is not correct.

i would personnly do this in a streaming fashion.
scan the rendered template line by line and skip newlines if the previous line was a new line
so i would not use replace or similar regex approaches

i like the idea of being able to pass a object via an interface to the function but
if we take that approach i think a simple line striping functor object should be added to lib common since i suspect that will be what people normally want.

we could do this in the nova operator today but we wanted to do it in lib common to benefit all the operators

at the end of the day this is a cosmetic enhancement but its sill nice to have for readablity

[auniyal61]

its still not looking good;
output confs https://logserver.rdoproject.org/55/855/79661db75220177782ddf4f496cb24c008b1df0f/github-check/nova-operator-tempest-multinode/4209eeb/controller/ci-framework-data/logs/openstack-k8s-operators-openstack-must-gather/namespaces/openstack/secrets/nova/

[stuggi]

ack, thanks for the explanation. doing this in the template can be confusing. I am ok to add it as a post processing, but I think it should be optional so that we keep the current behavior as the default and one needs to enable it if they like.

[SeanMooney]

something like this i think would work

https://go.dev/play/p/iOQ5UJ_RpvW?v=goprev.go?download=true.go?download=true.go?download=true.go?download=true

[auniyal61]

I was sure this time it would have worked, and printed like this: https://privatebin.corp.redhat.com/?9a9e2b43f5bf87bd#AyXQa1E1amv7vpGFbKeQGUKNu99ZNdN3t1JBHAb2Wppx

but its notcomming correctly
https://logserver.rdoproject.org/57/857/e3248dbf95dfb1878d852beef4475bfb68f4e811/github-check/nova-operator-tempest-multinode/616a00d/controller/ci-framework-data/logs/openstack-k8s-operators-openstack-must-gather/namespaces/openstack/secrets/nova/nova-api-config-data.yaml-01-nova.conf

[auniyal61]

cleanup worked
cleanup worked
https://logserver.rdoproject.org/57/857/0b3a0d59ae6b1ae5c0bba96da969a76b5fabd90b/github-check/nova-operator-tempest-multinode/0cbc344/controller/ci-framework-data/logs/openstack-k8s-operators-openstack-must-gather/namespaces/openstack/secrets/nova/nova-api-config-data.yaml-01-nova.conf

[gibizer]

I've pushed an extra commit to the branch with unit test for the cleaning logic and that shows some inconcistencies around the empty line handling between sections.

[gibizer]

I still prefer passing in the function references to run as post processing as that allows for a lot more flexibility in the future.

But if the others wants a separate boolean for each future post processing then at least be super specific what this boolean enables. The code Sean provided not just remove extra empty lines from the rendered template but also insert missing empty lines between ini like sections.

[gibizer]

We had a call with @auniyal61 and I learned that a single Template instance can actually mean a list of rendered files. Both via AdditionalTemplate and the logic that picks up a set of files from a path + glob. This means that a single PostProcessCleanup bool is actually applied not to a single file but to a set of diverse files(potentially a mix of different file types like ini, kolla json, apache conf). This would also be true for the set of functions as well if we change this to that type. This makes the situation more complicated as post processing cannot be configured per template file. Changing the Template to allow per file post processing is seems to be a big work so I drop the idea out of the scope of this change.

@stuggi How do you feel about eventually refactoring Template? I see three possible directions we could go

1. keep a single Template struct to pick up multiple files and render them as today, and then extend the interface to make such rendering still configurable per template file level
2. break the coupling between template rendering and Secret / CM creation with the rendered template. Make it two separate, client callable, steps. Like below to allow post processing on the client side.

   renderedTemplates = utils.RenderTemplate(template utils.Template)
   // the service operator can do post processing here on the client side
   utils.CreateCM/Secret(renderedTemplates)
   

3. break the current Template struct to have a list of Template structs one for each template file making the template rendering configuration per template file struct.

[auniyal61]

Hi @stuggi , can you please suggest on this.

[stuggi]

@auniyal61 sorry for the delay. I was out last week. I'll get back to this asap.

[stuggi]

@gibizer @SeanMooney @auniyal61 sorry for the delay, I finally had some time today to look into this. What you think about this approach [1]. This adds new template functions , especially for our use case here execTempl and removeNewLines. With this you can define your template inline and then render the template using the execTempl and remove multiple empty lines using removeNewLines. An example on how you could use that in a template is shown in the description.

Specific for the nova.conf in this example PR [2]. The benefit is that you could control this just from inside the template and no other code change is required.

[1] #569
[2] openstack-k8s-operators/nova-operator#874

[SeanMooney]

so we could likely proceed with this version for now

however how i would approach this is as follows

1 defince a new interface with one function which is takes a string and returns a string to post_process the rendered file.

instead of adding a bool here we would add a map of file exteion to interface object

then we would loop over all the files in the template and if there is registered post processor for that exteion in the map we can apply it to that specific file

we could also have a sentenal value for a default handleler if we want but i think that is likely not needed.

[gibizer]

I guess that is close to my suggestion/question 1 above. #557 (comment)

[SeanMooney]

by hte way the key could be a regex instead of an extnetion allowing mapping the handler to either a file or set of files.

this would just be applied to the keys for the config map/serecte but that might be overkill.

its simple to do and reason about however so we might just want to do it now

[SeanMooney]

yep more or less

i think breaking up the templates and configmap logic ectra will be a lot more work

we can do that eventually but for now i think this would be a minimal change to the existing proposal that we can extend with minimal impact on other operators

and in the future if we want to do that larger refactor for 2/3 we still can

i would do that by introducing a new Struct by the way and keeping but deprecating the current one so we can avoid a hard switch over.

so for the short term i would vote for 1 and then see if we need to do 2/3 later

[auniyal61]

closing in favor of #569