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 substitution with options #418

Merged
merged 5 commits into from
Jun 21, 2023
Merged

Conversation

srebhan
Copy link
Contributor

@srebhan srebhan commented Jun 16, 2023

This PR introduces the SubstituteWithOptions() function including options for custom patterns, custom substitution functions and a new replacement function for allowing to customize the replacement behavior if e.g. no mapping was found.
The existing SubstituteWith function is switched over to the newly introduced function.

@srebhan srebhan requested a review from ndeloof as a code owner June 16, 2023 17:18
@ndeloof
Copy link
Collaborator

ndeloof commented Jun 16, 2023

as there's no issue associated with this PR, can you please clarify how you expect this to be used ? Any upstream project relying on this ?

@srebhan
Copy link
Contributor Author

srebhan commented Jun 20, 2023

@ndeloof we are using this library for Telegraf and have to reproduce a specific behavior namely to only substitute environment-variable expressions if those environment variables are defined. This requirement comes from keeping backward compatibility to not break existing configurations. The issue linked above shows how we use this new function in code.

The intension is to be able to better control the replacement process while keeping the nice shell-like interpolation functionality.

Please let me know if I should open a corresponding issue!

@ndeloof
Copy link
Collaborator

ndeloof commented Jun 20, 2023

For legal reasons we require commits to be signed-off, please amend your commit and force push

Signed-off-by: Sven Rebhan <[email protected]>
@srebhan srebhan force-pushed the template_options branch from da7daeb to e329b70 Compare June 20, 2023 07:10
@srebhan srebhan requested a review from ndeloof June 20, 2023 10:14
Copy link
Collaborator

@glours glours left a comment

Choose a reason for hiding this comment

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

Is it possible to add an unit test checking the correct behavior of SubstituteWithOptions with a replacement function, please?

Copy link
Collaborator

@glours glours left a comment

Choose a reason for hiding this comment

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

Just a small modification in your test and we're ready to merge the PR

template/template_test.go Outdated Show resolved Hide resolved
Signed-off-by: Sven Rebhan <[email protected]>
Co-authored-by: Guillaume Lours <[email protected]>
@srebhan srebhan force-pushed the template_options branch from 0ba2e87 to e95c1cf Compare June 21, 2023 07:16
@glours glours merged commit 02dae79 into compose-spec:master Jun 21, 2023
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.

3 participants