-
-
Notifications
You must be signed in to change notification settings - Fork 121
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
base: master
Are you sure you want to change the base?
Conversation
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.
@bastelfreak could you please have a look? |
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.
${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.
@kenyon Thank for you review, i have done the requested changes , could you please review it? |
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.
Still won't work. epp('kafka/log4j.properties.epp', ...
needs the log_dir variable added.
@kenyon done, please have a look. |
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.
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.
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.
Has merge conflicts that need to be resolved.
Pull Request (PR) description
This Pull Request (PR) fixes the following issues