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 support for auto line indentation of multi line config to ConfigRenderer #445

Merged
merged 13 commits into from
Feb 5, 2025

Conversation

inf17101
Copy link
Contributor

@inf17101 inf17101 commented Jan 31, 2025

The PR fixes expansion of multi-line configuration items with no indentation level consideration when using the default handlebars template syntax.

Scenario:
A user defines a multi-line sharable configuration item in the state and uses this configuration item in a workload field where line indentation must be considered in the current context during expansion of templates. The indentation level shall be considered to ensure validity of indentation-sensitive formats like YAML.

Changes:

  • register a custom partial named indent in the handlebars template engine object (when rendering partials the line indentation will be automatically considered according to the current render context)
  • prevent escaping special character sequences like quotes \" to &quot, ...
  • user documentation about sharable configuration with multi-line content

The added system test shows an example use case with podman kube's ConfigMaps where it is important to keep line indentation while expanding templates in workload configuration.

Definition of Done

The PR shall be merged only if all items mentioned in CONTRIBUTING.md have been followed. In case an item is not applicable as described, please provide a short explanation in the description.

@inf17101 inf17101 added the enhancement New feature or request. Issue will appear in the change log "Features" label Jan 31, 2025
@inf17101 inf17101 requested a review from krucod3 January 31, 2025 14:07
@inf17101 inf17101 changed the title Add support for keeping line indentation of multi line config to ConfigRenderer Add support for auto line indentation of multi line config to ConfigRenderer Jan 31, 2025
Copy link
Contributor

@krucod3 krucod3 left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of small things.

server/doc/swdesign/README.md Outdated Show resolved Hide resolved
server/src/ankaios_server/config_renderer.rs Outdated Show resolved Hide resolved
@inf17101
Copy link
Contributor Author

inf17101 commented Feb 3, 2025

Looks good, just a couple of small things.

Thank you for the review. I will fix the findings. In addition, I will add the new partial syntax to the user documentation.

@krucod3
Copy link
Contributor

krucod3 commented Feb 3, 2025

Is this a standalone PR, or is it part of the current ticket #393? I was assuming the later, but if we want it as a standalone one, we should either add a description to it, or an create an issue and link it. (I would go for the description.)

@inf17101
Copy link
Contributor Author

inf17101 commented Feb 3, 2025

Is this a standalone PR, or is it part of the current ticket #393? I was assuming the later, but if we want it as a standalone one, we should either add a description to it, or an create an issue and link it. (I would go for the description.)

The issue was detected within #393 but the fix is independent and a general configuration rendering issue. I will add a description.

Copy link
Contributor

@krucod3 krucod3 left a comment

Choose a reason for hiding this comment

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

Reviewed, small findings documented.

There are also some value duplications in the new test for the escaped string, but it's ok for a test.

doc/docs/usage/manifest/config-rendering.md Outdated Show resolved Hide resolved
doc/docs/usage/manifest/config-rendering.md Outdated Show resolved Hide resolved
@inf17101
Copy link
Contributor Author

inf17101 commented Feb 4, 2025

Reviewed, small findings documented.

There are also some value duplications in the new test for the escaped string, but it's ok for a test.

I will make constants but not global, because it is only needed inside the escape test.

@inf17101
Copy link
Contributor Author

inf17101 commented Feb 4, 2025

Reviewed, small findings documented.
There are also some value duplications in the new test for the escaped string, but it's ok for a test.

I will make constants but not global, because it is only needed inside the escape test.

Done.

Copy link
Contributor

@krucod3 krucod3 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@krucod3 krucod3 merged commit ba9afe0 into main Feb 5, 2025
10 checks passed
@krucod3 krucod3 deleted the add_autoindent_partial branch February 5, 2025 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request. Issue will appear in the change log "Features" ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants