-
Notifications
You must be signed in to change notification settings - Fork 50
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
Fixes whitespaces and newline in nova conf template #842
Fixes whitespaces and newline in nova conf template #842
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: auniyal61 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/9169b5f8dc614120bf2905d657c94b5c ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 31m 03s |
allow_resize_to_same_host = true | ||
{{end}} | ||
{{- end }} |
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.
This can work and it's what i originally tried and abandoned last year but I'm not sure we want to commit to doing this.
the other option that we briefly discussed was post-processing the template and removing any instance of multiple newlines with a single new line.
if we make this change using the template timing feature then i want an env test tests for this to assert we do not have multiple consecutive new lines to catch any template changes that may miss the trimming and reintroduce this issue.
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.
he other option that we briefly discussed was post-processing the template and removing any instance of multiple newlines with a single new line.
yeah I tried that earlier, replaice "\n\n\n+" with "\n".
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.
but I tried it in inside a test,
@SeanMooney can you please suggest if we go with this approach, at what place should we make final changes .
i.e once we are sure, final nova.conf(s) are ready to format.
I think the foratting is dones here, but not sure at what point/place nova-operator can update this.
https://github.com/openstack-k8s-operators/lib-common/blob/bd202c880706adfbcc64ee4a57e877503a274a69/modules/common/util/template_util.go#L138
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.
As the current output still has plenty of whitespace even after the template tunining I would suggest to try to reduce that as a post processing step as discussed here.
The utils.Template type we use to pass the template to lib-common already has optional flags like https://github.com/openstack-k8s-operators/lib-common/blob/0ae9f7f9e059a903b1c47119ed05aa509f8afb77/modules/common/util/template_util.go#L58 So I think it would be OK to add an optional postProcessing function callback to that type that gets the rendered text and return a new text. The default can be a simple identity that returns the text unchanged. Then nova-operator can hook into the lib-common code and do the whitespace reduction on the rendered text. (It would be even better to split up the monolithic secret.EnsureSecrets lib-common function that the template rendering and the secret creation can be composed on the client side.)
@auniyal61 you can put replace line into go.mod to depend on an unmerged lib-common patch to be able to show that the lib-common change works with nova-operator and to show how the config would look.
current error:
controlplane eventually get created |
5d44506
to
6d0f51c
Compare
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/5603e783fdc541a194db8ca611038722 ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 34m 58s |
Test failures are relevant:
|
cell0 db sync job fails probably due to the following config issue:
|
this is/will be done #857 |
This is a followup patch for #823