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

Enable passing prlog4j file #159

Merged
merged 5 commits into from
Jul 11, 2020
Merged

Conversation

kalap92
Copy link
Contributor

@kalap92 kalap92 commented Jul 7, 2020

Signed-off-by: Piotr Kala Piotr.Kala@pega.com

Signed-off-by: Piotr Kala <Piotr.Kala@pega.com>
@kalap92 kalap92 requested review from dcasavant and a team as code owners July 7, 2020 16:35
@dcasavant
Copy link
Member

@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 ).

@kalap92 kalap92 requested a review from taz-mon as a code owner July 7, 2020 22:20
@Emilio-Pega
Copy link

@dcasavant We don't mind if you change the impl, we just want it to function :). Your logic is much cleaner anyway.

@kalap92
Copy link
Contributor Author

kalap92 commented Jul 8, 2020

@dcasavant thanks - looks good to me.

@Emilio-Pega
Copy link

@taz-mon @dcasavant Can we get the PR merged and get a release with the change? This is blocking our current sprint.

@dcasavant
Copy link
Member

@taz-mon to review tomorrow and will merge after approval.
Anyone else from @pegasystems/deployment is welcome to take a look too.

@@ -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.
Copy link
Contributor

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.

@taz-mon
Copy link
Contributor

taz-mon commented Jul 10, 2020

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!

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.

4 participants