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

fix: Handle log4j2 not being yaml (#110) #118

Closed
wants to merge 3 commits into from
Closed

fix: Handle log4j2 not being yaml (#110) #118

wants to merge 3 commits into from

Conversation

sastorsl
Copy link
Contributor

@sastorsl sastorsl commented Nov 2, 2021

Bump chart (#110)

Signed-off-by: Stein Arne Storslett sastorsl@users.noreply.github.com

Description

Render log4j2.properties properly when added to values.yaml / config.
The current version does a toYaml, but log4j2 is key = value based and not yaml so without this PR log4j2 will not be rendered properly (see issue for details).

Maintainers may adjust the Chart version as needs be, ie. to align with other PR's but please contact me for any other changes.

Issues Resolved

#110

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Bump chart (#110)

Signed-off-by: Stein Arne Storslett <sastorsl@users.noreply.github.com>
@peterzhuamazon
Copy link
Member

Looks like version bump needs to be higher?

@DandyDeveloper
Copy link
Collaborator

@sastorsl Wouldn't this be better to just tpl <Value> . rather then this? The toYaml may be redundant.

@sastorsl
Copy link
Contributor Author

sastorsl commented Nov 3, 2021

Looks like version bump needs to be higher?

A bit hard to determine from the outside, given that there are a few PR's, and thus conflicts.
@peterzhuamazon could a solution be that you in the team name a version number and I'll add it?

@TheAlgo
Copy link
Member

TheAlgo commented Nov 3, 2021

Seeing the current scenario I feel we can start off with long lived release branches. If everyone is okay with it I can create one. We can have those branches merged to main after the patch is ready so one and so forth.

Reference : #79

@sastorsl
Copy link
Contributor Author

sastorsl commented Nov 3, 2021

@sastorsl Wouldn't this be better to just tpl <Value> . rather then this? The toYaml may be redundant.

@DandyDeveloper I was really trying to not change how the chart handled config files, trying to avoid those regressions...
The PR I submitted "only" changes the handling of log4j2.properties and thus does not change anything else.

tpl docs are quite terse, and I see that there has been some issues with regards to tpl and the use of range, i.e. helm/helm#5979
I did a couple of tests and was at least not able to find a solution in a satisfactory time frame.

I would think (hope) that rewriting to use tpl could be in scope further down the road for somebody more well versed in it's use, and I hope that we can move forward with this PR as it stands.

@peterzhuamazon
Copy link
Member

@sastorsl DCO check fail because pulling from main will not sign.
You need to sign again after pulling.

Thanks.

@sastorsl
Copy link
Contributor Author

sastorsl commented Nov 3, 2021

@peterzhuamazon I've resolved the DCO-issue now.

@peterzhuamazon
Copy link
Member

For some reason it is keep pointing to 5a963b9daeaf3d89f5856366df1f6e8fc730e4ba

sastorsl and others added 2 commits November 3, 2021 22:19
Signed-off-by: Stein Arne Storslett <sastorsl@users.noreply.github.com>
Signed-off-by: TheAlgo <dhirajjain0@gmail.com>
@sastorsl
Copy link
Contributor Author

sastorsl commented Nov 3, 2021

I'm removing this PR and replacing it with a clean one (#123).
Sorry for the mess...

@sastorsl sastorsl closed this Nov 3, 2021
@sastorsl sastorsl deleted the opensearch-configmap-log4j branch November 3, 2021 21:31
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.

[BUG][opensearch] log4j2.properties in values.yaml does not work with toYaml in configmap.yaml
4 participants