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

Update log4j.properties.epp #339

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Kumar-Test
Copy link

@Kumar-Test Kumar-Test commented Nov 11, 2022

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

We were using this module for our project but it does not accept the parameter kafka.logs.dir since it is not initialised in the param.pp. Either you include it or accept this change.
@Kumar-Test
Copy link
Author

@bastelfreak could you please have a look?

Copy link
Member

@kenyon kenyon left a comment

Choose a reason for hiding this comment

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

${log_dir} won't be replaced with its value unless it's in EPP templating delimiters <%= %>, and the variable is passed to the template in the epp() function, or the variable is fully-qualified. Also we need #336 so that the tests pass.

@Kumar-Test Kumar-Test requested a review from kenyon November 13, 2022 10:01
@Kumar-Test
Copy link
Author

@kenyon Thank for you review, i have done the requested changes , could you please review it?

Copy link
Member

@kenyon kenyon left a comment

Choose a reason for hiding this comment

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

Still won't work. epp('kafka/log4j.properties.epp', ... needs the log_dir variable added.

@Kumar-Test
Copy link
Author

@kenyon done, please have a look.

Copy link
Member

@kenyon kenyon left a comment

Choose a reason for hiding this comment

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

Should be OK now, but we need #336 done so that tests pass. Also would be good to have this log_dir change covered by tests.

@TheMeier TheMeier requested a review from kenyon April 17, 2024 09:39
Copy link
Member

@kenyon kenyon left a comment

Choose a reason for hiding this comment

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

Has merge conflicts that need to be resolved.

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.

3 participants