fix(bug): Inconsistency when merging fragments with same file name with different profiles#3803
fix(bug): Inconsistency when merging fragments with same file name with different profiles#3803ash-thakur-rh wants to merge 22 commits intoeclipse-jkube:masterfrom
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3803 +/- ##
=============================================
+ Coverage 59.36% 72.20% +12.84%
- Complexity 4586 4871 +285
=============================================
Files 500 494 -6
Lines 21211 19424 -1787
Branches 2830 2588 -242
=============================================
+ Hits 12591 14026 +1435
+ Misses 7370 4192 -3178
+ Partials 1250 1206 -44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ash-thakur-rh
requested review from
manusa,
rohanKanojia and
sunix
as code owners
manusa
left a comment
manusa
left a comment
There was a problem hiding this comment.
As agreed internally, prior to tackling this PR, we need to add the relevant fix to ensure that jkube-temp is empty before the resource target/goal is started.
Already taken care in this change with the configuration of ResourceServiceConfig. Is it okay to handle this in this PR, or we handle it in a separate PR? |
The idea was to handle this in a separate PR to ensure that nothing breaks with regards to cleaning up the jkube-temp dirctory. |
ash-thakur-rh
force-pushed
the
fix-multi-env-name-resource
branch
from
9e65879 to
1e0cca3
Compare
| new File[]{file2}, | ||
| outDir, | ||
| (source, target) -> yaml2 | ||
| ); |
There was a problem hiding this comment.
This is very confusing, the signature of the method suggests that multiple files can be processed but in the test we're running the processor once for each file.
The Javadoc also suggests that this method should only be called once per batch of files
| // When - Process first file | ||
| ResourceFileProcessor.processFiles( | ||
| new File[]{file1}, | ||
| outDir, | ||
| (source, target) -> "content1" | ||
| ); | ||
|
|
||
| // When - Process second file with same name (should overwrite, not merge) | ||
| File[] result = ResourceFileProcessor.processFiles( | ||
| new File[]{file1}, | ||
| outDir, | ||
| (source, target) -> "content2" | ||
| ); |
| private static boolean isEffectivelyEmpty(String yaml) { | ||
| if (yaml == null || yaml.trim().isEmpty()) { | ||
| return true; | ||
| } | ||
| // Remove comments and whitespace to check if there's any actual content | ||
| String[] lines = yaml.split("\n"); | ||
| for (String line : lines) { | ||
| String trimmed = line.trim(); | ||
| // Skip empty lines, comments, and YAML document separator | ||
| if (!trimmed.isEmpty() && !trimmed.startsWith("#") && !trimmed.equals("---")) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This looks brittle. I see this is used in the mergeYaml method.
Why not simply deserialize and check if the deserializes object is not empty.
| private File[] getFiles(File[] resourceFiles, File outDir) throws IOException { | ||
| return ResourceFileProcessor.processFiles(resourceFiles, outDir, (resource, targetFile) -> | ||
| interpolate(resource, kubernetesExtension.javaProject.getProperties(), | ||
| kubernetesExtension.getFilter().getOrNull()) | ||
| ); | ||
| } |
There was a problem hiding this comment.
This method is probably not necessary, just move the content of this method to line 227
jkube-kit/common/src/main/java/org/eclipse/jkube/kit/common/util/ResourceFileProcessor.java
Outdated
Show resolved
Hide resolved
# Conflicts: # CHANGELOG.md
ash-thakur-rh
force-pushed
the
fix-multi-env-name-resource
branch
from
1e0cca3 to
3a2dde2
Compare
| Object deserialized = YAML_MAPPER.readValue(yaml, Object.class); | ||
| return isDeserializedObjectEmpty(deserialized); |
There was a problem hiding this comment.
| Object deserialized = YAML_MAPPER.readValue(yaml, Object.class); | |
| return isDeserializedObjectEmpty(deserialized); | |
| return YAML_MAPPER.readValue(yaml, Map.class).isEmpty(); |
|
3 participants
Description
Fixes #2758
Type of change
test, version modification, documentation, etc.)
Checklist