-
Notifications
You must be signed in to change notification settings - Fork 75
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(charts): fix indentation for JAVA_OPTIONS in deployments. #8416
Conversation
Thank you @saernz ! We will review this PR. |
We would truly appreciate it if you can sign your commits and DCO basically use the |
Signed-off-by: saernz <15317826+saernz@users.noreply.github.com>
Sorry, should be all good now. Was not too keen to use my email but I found out how github does it privately. |
I think as per my reasoning in the resolved requested change we should probably be able to move ahead with this now. I'm not sure if there is a way to run a test release for the helm charts to see what happens? As per my comments in the requested change, I believe there is something in the helm chart release pipeline that fixes the indentation of the raw source files, so even though the JAVA_OPTIONS are only indented by 10 in the raw source, once they pass through that pipeline, they become indented by 12. This is because the parent array of containers is indented differently in the raw helm templates in GitHub from what comes out the other side in the released helm chart where it has +2 spaces for array items. You can unzip the released helm charts and compare the differences from there. I think whatever is happening to cause that is probably better fixed in another issue, though for now this fix should at least maintain the status quo. |
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.
I think @saernz is right and I believe it's coming from a source that's using a windows computer 🫢
Fix indentation for JAVA_OPTIONS in deployments. Signed-off-by: saernz <15317826+saernz@users.noreply.github.com> Co-authored-by: saernz <15317826+saernz@users.noreply.github.com> Co-authored-by: Mohammad Abudayyeh <47318409+moabu@users.noreply.github.com> Former-commit-id: ae3999f
Description
Target issue
Fixes #8415
Closes #8415
If someone else could test this fixes the issue that would be great. I've so far tested it for the config-api by manually editing my janssen charts folder with the change, which fixed the issue for me. I haven't tested any of the other deployments.