refactor (jkube-kit/build) : Move buildArgs related logic to a new class BuildArgResolverUtil #2994
Conversation
This file contains 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
Eclipse JKube CI ReportStarted new GH workflow run for #2994 (2024-04-30T12:52:38Z) ⚙️ JKube E2E Tests (8894491613)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2994 +/- ##
=============================================
+ Coverage 59.36% 70.85% +11.49%
- Complexity 4586 5080 +494
=============================================
Files 500 490 -10
Lines 21211 19571 -1640
Branches 2830 2525 -305
=============================================
+ Hits 12591 13867 +1276
+ Misses 7370 4478 -2892
+ Partials 1250 1226 -24 ☔ View full report in Codecov by Sentry. |
manusa
reviewed
Apr 29, 2024
Comment on lines
33
to
35
| public static Map<String, String> mergeBuildArgs(ImageConfiguration imageConfig, JKubeConfiguration configuration) { | ||
| return prepareBuildArgs(addBuildArgs(configuration), imageConfig.getBuildConfiguration()); | ||
| } |
There was a problem hiding this comment.
Could we use this PR to provide some sort of order and orchestration for this method instead of just chaining methods.
It'd be nice if the method would show how the args are computed and populated based on their source. This would also show how the precedence works.
final Map<String, String> args = new HashMap<>();
// Least precedence args added to the map
// Next args added to the map, overriding any previous entries
// ...
return args;
There was a problem hiding this comment.
While addressing this, I noticed that we're using Guava's com.google.common.collect.ImmutableMap that throws exception when colliding keys are added.
Shall I remove its usage? At the moment, we are throwing exception on collision.
There was a problem hiding this comment.
So at the moment it's not possible to override args? that's weird.
We should definitely remove Guava.
If the current behavior is that override is not possible, we can preserve it. We should also create a new issue to see if we want to change this. IMO users my want to be able to provide one-off overrides, so maybe a warning instead of an exception is better.
There was a problem hiding this comment.
I've created #3000 for this task.
18 tasks
rohanKanojia
force-pushed
the
pr/refactor-build-args-util-class
branch
from
a34d5da
to
06017bf
Compare
2 tasks
…ass BuildArgResolverUtil Related to eclipse-jkube#2904 It's better to move code for resolving build args from different sources to a dedicated class. It doesn't look like BuildService is the right place for this. This way we can also use it from other BuildServices of other build strategies. Signed-off-by: Rohan Kumar <rohaan@redhat.com>
rohanKanojia
force-pushed
the
pr/refactor-build-args-util-class
branch
from
06017bf
to
e4b5075
Compare
Simplify logic to allow for easier future developments and improvements. Signed-off-by: Marc Nuri <marc@marcnuri.com>
manusa
force-pushed
the
pr/refactor-build-args-util-class
branch
from
af39006
to
bb50a22
Compare
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Related to #2904
It's better to move code for resolving build args from different sources to a dedicated class. It doesn't look like BuildService is the right place for this. This way we can also use it from other BuildServices of other build strategies.
This PR just moves BuildArg related logic to a new utility class. There is one additional test for adding build args from docker proxy config.
Type of change
test, version modification, documentation, etc.)
Checklist