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

Doc: Environment variables cannot be in config.string comments. #16689

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mashhurs
Copy link
Contributor

@mashhurs mashhurs commented Nov 18, 2024

Release notes

[rn:skip]

What does this PR do?

Adds a limitation where environment ${VAR}s cannot be used in config.string. To avoid pipeline run config failure, $ can be removed.

Why is it important/What is the impact to the user?

From functional point of view, users are not impacted.

Checklist

  • [ ] My code follows the style guidelines of this project
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files (and/or docker env variables)
  • [ ] I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@mashhurs mashhurs added the docs label Nov 18, 2024
@mashhurs mashhurs requested review from jsvd and karenzone November 18, 2024 23:47
@mashhurs mashhurs mentioned this pull request Nov 18, 2024
3 tasks
@donoghuc
Copy link
Member

Perhaps I am misunderstanding the issue but I thought that the problem here is that interpolation occurs regardless of whether the interpolation site is within a comment or not due to how the template is generated. This can lead to some somewhat confusing behavior where commented out sections still need values for interpolated values. The root of the issue is just the order of operations for interpolation in turning the information from template to instruction. SO instead of saying interpolation is not allowed instead should we describe the behavior and note that if you have interpolations anywhere in the string you need to ensure you have provided values?

@mashhurs
Copy link
Contributor Author

Perhaps I am misunderstanding the issue but I thought that the problem here is that interpolation occurs regardless of whether the interpolation site is within a comment or not due to how the template is generated. This can lead to some somewhat confusing behavior where commented out sections still need values for interpolated values. The root of the issue is just the order of operations for interpolation in turning the information from template to instruction. SO instead of saying interpolation is not allowed instead should we describe the behavior and note that if you have interpolations anywhere in the string you need to ensure you have provided values?

I wonder what benefits does end user get if we explain the interpolation behavior.
From the end user experience, my practical approach here, user has mostly a running pipeline, later on comments env ${VAR} of the config.string. And once LS starts, the user faces an error and start searching context for using env ${VAR} in comments.

docs/static/env-vars.asciidoc Outdated Show resolved Hide resolved
Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

Suggested a few tweaks to both sentences.

Suggestion to revise the statements.

Co-authored-by: João Duarte <[email protected]>
@mashhurs mashhurs requested a review from jsvd December 2, 2024 17:33
Copy link
Contributor

github-actions bot commented Dec 2, 2024

📃 DOCS PREVIEWhttps://logstash_bk_16689.docs-preview.app.elstc.co/diff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Commented out variable substitutions may still be evaluated in pipelines.yml
3 participants