Draft
Conversation
Member
Eclipse JKube CI ReportStarted new GH workflow run for #2689 (2024-02-23T20:35:04Z) ⚙️ JKube E2E Tests (8024533426)
|
Jurrie
force-pushed
the
feature/Allow_plain_Helm_in_yaml_fragments
branch
2 times, most recently
from
9ab8fdb to
fe84181
Compare
Jurrie
changed the title
Jurrie
commented
Feb 17, 2024
|
|
||
| /** | ||
| * Ported from https://github.com/fabric8io/fabric8-maven-plugin/commit/d6bdaa37e06863677bc01cefa31f7d23c6d5f0f9 | ||
| * This function will replace all Helm directives with a valid Yaml line containing the base64 encoded Helm directive. |
Member
|
Thanks a lot for looking into this 🙌 Didn't give a thorough look, but it seems that with this PR we could finally mix in invalid Helm/Go template syntax withing the JKube-processed Kubernetes resources (those stored at src/main/jkube). I'm not sure if we're considering here corner cases such as Regarding the PR, I think it probably needs proper breaking down so that it's easier to evaluate each of the things addressed here:
|
17 tasks
Contributor
Author
|
Hi @manusa, thanks for your reply.
That's the intention at least 😉
I agree on the regex part. Also, in my opinion regex's are less maintainable than regular code. I'll see what I can do to convert this to use regular string parsing. The corner case that you mention - is that even valid? I'll try to include such a corner case in the unit test.
Fine by me to break this down. The part "values.yaml sorting without modifying the global Jackson configuration" is already merged as PR #2702. As for the other two parts: I'm not sure if they stand on their own. In my opinion that should be a single PR. If you disagree, could you explain how you would like to see those two PRs? Also, I noted in the description:
Do you feel comfortable to merge this feature anyway? |
Signed-off-by: Jurrie Overgoor <eclipse.org@jurr.org>
Signed-off-by: Jurrie Overgoor <eclipse.org@jurr.org>
Jurrie
force-pushed
the
feature/Allow_plain_Helm_in_yaml_fragments
branch
from
fe84181 to
0bfaf28
Compare
Member
Once this PR is rebased I'll probably have a clear view. During my initial review, I saw you changed and added methods to the generic Serialization tools, this is what I properly need to understand.
Haven't checked your implementation yet, but the idea for the inner-loop (k8s:resource) would be to ignore the helm-specific sections and warn the user about it. |
Contributor
Author
The PR is rebased. The basic idea behind the changes in Serialization is to reroute all methods that have different type inputs to a single method that does marshalling / unmarshalling. That's the point where I do the "mask the Helm directives" / "unmask the Helm directives".
That is not what is happening in this pull request currently. The Helm directives will still be there in the final output. I do have a solution to this though. Maybe it's best to first introduce the goals |
Contributor
Author
In 0087d30 the escaping logic is rewritten. I tried to use as little regex as possible. Hopefully the new code is better maintainable and more robust.
The corner case you mentioned ( Another note is that sometimes the Helm lines are re-ordered and appear on the bottom of a Yaml block. This seems to be because of the builders that are used to parse the Yaml files. The builders differentiate between Yaml directives they know in their context, and |
Signed-off-by: Jurrie Overgoor <eclipse.org@jurr.org>
Jurrie
force-pushed
the
feature/Allow_plain_Helm_in_yaml_fragments
branch
from
88d310e to
0087d30
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR contains preliminary support to allow users to have Helm in their Yaml fragments.
There are still things left to do, but I wanted to get an overall review before I continue:
Type of change
test, version modification, documentation, etc.)
Checklist