fix (jkube-kit/enricher) : WellKnownLabelEnricher also considers labels added via resource configuration #2587
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 #2587 (2024-02-05T10:42:25Z) ⚙️ JKube E2E Tests (7783086545)
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2587 +/- ##
=============================================
+ Coverage 59.36% 70.38% +11.01%
- Complexity 4586 4969 +383
=============================================
Files 500 480 -20
Lines 21211 19342 -1869
Branches 2830 2496 -334
=============================================
+ Hits 12591 13613 +1022
+ Misses 7370 4501 -2869
+ Partials 1250 1228 -22 ☔ View full report in Codecov by Sentry. |
rohanKanojia
force-pushed
the
pr/well-known-label-regression-resourceconfig
branch
2 times, most recently
from
dc23e62 to
a08fbcd
Compare
manusa
reviewed
Jan 31, 2024
Comment on lines
166
to
179
| private Map<String, String> convertLabelsToMap(Supplier<Properties> labelSupplier) { | ||
| Properties labelsAsProps = labelSupplier.get(); | ||
| Map<String, String> labels = new HashMap<>(); | ||
| if (labelsAsProps != null) { | ||
| for (Map.Entry<Object, Object> e : labelsAsProps.entrySet()) { | ||
| String key = (String) e.getKey(); | ||
| String value = (String) e.getValue(); | ||
| labels.put(key, value); | ||
| } | ||
| } | ||
| return labels; | ||
| } | ||
|
|
||
| private Map<String, String> mergedSelectors(Map<String, String> originalSelectors, List<Supplier<Properties>> labelSupplierList) { | ||
| Map<String, String> labelsFromResourceConfig = extractLabelsConfiguredViaResourceConfig(labelSupplierList); | ||
| MapUtil.mergeIfAbsent(originalSelectors, createLabels(true, labelsFromResourceConfig)); | ||
| return originalSelectors; | ||
| } |
There was a problem hiding this comment.
Have you checked PropertiesUtil and MapUtil to see if some of the provided methods can be reused?
I'm kind of sure we can easily remove the convertLabelsToMap method and replace it with toMap(labelSupplier.get()), but we can very likely simplify more
There was a problem hiding this comment.
mergeMaps + toMap should really allow you to remove extractLabelsConfiguredViaResourceConfig + convertLabelsToMap and make it more clear in the calling methods.
I'm not sure if we're being clear about precedence either.
Also MapUtil.mergeMaps can be changed to MapUtil.mergeMaps(Map<K, V> maps...) to be able to support a variable number of maps
rohanKanojia
force-pushed
the
pr/well-known-label-regression-resourceconfig
branch
from
a08fbcd to
ff384ed
Compare
manusa
reviewed
Jan 31, 2024
| private Map<String, String> mergedSelectors(Map<String, String> originalSelectors) { | ||
| MapUtil.mergeIfAbsent(originalSelectors, createLabels(true)); | ||
| @SuppressWarnings("unchecked") | ||
| private Map<String, String> mergedSelectors(Map<String, String> originalSelectors, List<Supplier<Properties>> labelSupplierList) { |
There was a problem hiding this comment.
I'm still not sure why we're using Supplier here instead of providing the properties themselves. Is there any reason to make this lazy?
manusa
reviewed
Jan 31, 2024
Comment on lines
50
to
54
| * @param <K> first type | ||
| * @param <V> second type |
There was a problem hiding this comment.
Should be changed.
We might also want to comment on the precedence of the maps
There was a problem hiding this comment.
Are you referring to @param entries for types?
rohanKanojia
force-pushed
the
pr/well-known-label-regression-resourceconfig
branch
from
ff384ed to
bebe736
Compare
…ls added via resource configuration While adding labels and selectors to a resource, WellKnownLabelEnricher and ProjectLabelEnricher would give precedence to label configured via resource label XML/Groovy DSL configuration. This precedence would depend on the type of resource label configuration used. If `all` has been provided in configuration, this means precedence would be given for all resources. However, if some specific resource label configuration is used; precedence would only be given for that specific resource Signed-off-by: Rohan Kumar <rohaan@redhat.com>
rohanKanojia
force-pushed
the
pr/well-known-label-regression-resourceconfig
branch
from
bebe736 to
6e214f2
Compare
Signed-off-by: Marc Nuri <marc@marcnuri.com>
manusa
force-pushed
the
pr/well-known-label-regression-resourceconfig
branch
from
2fc40ac to
420c2ec
Compare
|
manusa
approved these changes
Feb 5, 2024
rohanKanojia
deleted the
pr/well-known-label-regression-resourceconfig
branch
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
Fix #1700
While adding labels and selectors to a resource, WellKnownLabelEnricher and ProjectLabelEnricher would give precedence to label configured via resource label XML/Groovy DSL configuration.
This precedence would depend on the type of resource label configuration used. If
allhas been provided in configuration, this means precedence would be given for all resources. However, if some specific resource label configuration is used; precedence would only be given for that specific resourceMap<String, String> labelsViaResourceConfigin AbstractLabelEnricher.createLabels to accommodate labels configured via resource configuration.labelsViaResourceConfigwould override default opinionated or enricher configuration settings.createLabels, various resource visitors would also pass their specific resource label configuration option so that that can be checked while creating opinonated labelsType of change
test, version modification, documentation, etc.)
Checklist