fix: images can be configured with properties #3042
Merged
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 #3042 (2024-05-14T12:19:42Z) ⚙️ JKube E2E Tests (9078134825)
|
manusa
force-pushed
the
feat/container-image-properties
branch
3 times, most recently
from
2c65b16
to
6f62454
Compare
manusa
force-pushed
the
feat/container-image-properties
branch
from
6f62454
to
b62df78
Compare
manusa
force-pushed
the
feat/container-image-properties
branch
2 times, most recently
from
a734084
to
9a4de07
Compare
17 tasks
manusa
force-pushed
the
feat/container-image-properties
branch
2 times, most recently
from
a2eebd7
to
19af35d
Compare
manusa
commented
May 13, 2024
| @@ -65,20 +65,16 @@ images: | |||
| - name: "registry/image:tag" | |||
| build: | |||
| contextDir: "@endsWith('extension-configuration/src/main/context-dir')@" | |||
| dockerFile: "@endsWith('extension-configuration/src/main/context-dir/Dockerfile')@" | |||
There was a problem hiding this comment.
Previously, the images that were declared in the context of the KubernetesExtension class were mutated by the DefaultGeneratorManager.
In the current implementation, (unresolved) images passed in to the DefaultGeneratorManager are no longer mutated and are cloned and enriched incrementally instead.
This is reflected here, where the expected config should only show the values passed in by the user in the build.gradle configuration.
manusa
force-pushed
the
feat/container-image-properties
branch
from
19af35d
to
8ec834e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3042 +/- ##
=============================================
+ Coverage 59.36% 70.97% +11.60%
- Complexity 4586 5072 +486
=============================================
Files 500 488 -12
Lines 21211 19449 -1762
Branches 2830 2502 -328
=============================================
+ Hits 12591 13803 +1212
+ Misses 7370 4431 -2939
+ Partials 1250 1215 -35 ☔ View full report in Codecov by Sentry. |
|
|
||
| | *exportTargetDir* | ||
| | Specification whether the `targetDir` should be exported as a volume. This value is `true` by default except in the | ||
| case the `targetDir` is set to the container root (`/`). It is also `false` by default when a base image is used with | ||
| `from` since exporting makes no sense in this case and will waste disk space unnecessarily. | ||
| | `jkube.container-image.assembly.exportTargetDir` | ||
|
|
||
| | *excludeFinalOutputArtifact* |
There was a problem hiding this comment.
why is property not added for this field?
There was a problem hiding this comment.
It was not part of the original list of properties. We can certainly add it (Maybe as a follow up)
| .interval(valueProvider.getInteger(ConfigKey.WATCH_INTERVAL, config.getIntervalRaw())) | ||
| .postGoal(valueProvider.getString(ConfigKey.WATCH_POSTGOAL, config.getPostGoal())) | ||
| .postExec(valueProvider.getString(ConfigKey.WATCH_POSTEXEC, config.getPostExec())) | ||
| .modeString(valueProvider.getString(ConfigKey.WATCH_POSTGOAL, config.getMode() == null ? null : config.getMode().name())) |
There was a problem hiding this comment.
Shouldn't we be using ConfigKey.WATCH_MODE here ?
There was a problem hiding this comment.
Definitely, this seems to have been carried over from DMP:
https://github.com/fabric8io/docker-maven-plugin/blob/25e772ed663d34bf2932eb48baabd6492065b0cf/src/main/java/io/fabric8/maven/docker/config/handler/property/PropertyConfigHandler.java#L458
That should be fixed there too.
I'll change here as soon as you finish the review so I can push all the changes at once.
rohanKanojia
approved these changes
May 14, 2024
Signed-off-by: Marc Nuri <marc@marcnuri.com>
manusa
force-pushed
the
feat/container-image-properties
branch
from
8ec834e
to
d4990ec
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
Fixes #296
fix: images can be configured with properties
Type of change
test, version modification, documentation, etc.)
Checklist