-
Notifications
You must be signed in to change notification settings - Fork 206
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
Enable passing prlog4j file #159
Conversation
Signed-off-by: Piotr Kala <Piotr.Kala@pega.com>
@kalap92 Thanks for the submission! The proposed config template was in the root directory which wont be evaluated, so I moved it to the templates directory and updated the existing file. Hope you don't mind that I updated your branch (feel free to back me out). I also refactored the config file a bit. By defaulting the .config object to a variable you can effectively initialize it and avoid lots of the checks that have been building up to see if it exists or not. By refactoring that I was able to get the file size down by almost 50% and also (subjectively) made the logic blocks a bit more readable. Let me know what you think? The API contract in values.yaml is unchanged so it should be good, but can you try this out and make sure it still meets your use case? If this looks like a good change to you, the last thing to do is update the Pega charts readme to reflect this change (+ @taz-mon ). |
@dcasavant We don't mind if you change the impl, we just want it to function :). Your logic is much cleaner anyway. |
@dcasavant thanks - looks good to me. |
@taz-mon @dcasavant Can we get the PR merged and get a release with the change? This is blocking our current sprint. |
@taz-mon to review tomorrow and will merge after approval. |
charts/pega/README.md
Outdated
@@ -429,12 +429,30 @@ tier: | |||
|
|||
### Pega configuration files | |||
|
|||
While default configuration files are included by default, the Helm charts provide extension points to override them with additional customizations. To change the configuration file, specify a relative path to a local implementation to be injected into a ConfigMap. | |||
While default configuration files are included by default, the Helm charts provide extension points to override them with additional customizations. To change the configuration file, specify the replacement implementation to be injected into a ConfigMap. |
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.
While Pega includes default configuration files in the Helm charts, the charts provide extension points to override the defaults with additional customizations. To change the configuration file, specify the replacement implementation to be injected into a ConfigMap.
I left a review comment that would be good to add to the readme, but it's not required...only if it's easy to merge in my suggestion. Otherwise it looks great! |
Signed-off-by: Piotr Kala Piotr.Kala@pega.com