fix: ordered managed dependents #1120
Conversation
| @@ -22,7 +22,7 @@ | |||
| private ResourceEventFilter<R> customResourcePredicate; | |||
| private final ControllerConfiguration<R> original; | |||
| private Duration reconciliationMaxInterval; | |||
| private final Map<String, DependentResourceSpec<?, ?>> dependentResourceSpecs; | |||
| private final LinkedHashMap<String, DependentResourceSpec<?, ?>> namedDependentResourceSpecs; | |||
There was a problem hiding this comment.
Why use a map why not a list? as in ControllerConfiguration or DefaultControllerConfiguration: List<DependentResourceSpec<?, ?>>
DependentResourceSpec already contains the name. It's basically data duplication.
Would be also consistent with those 2 classes.
There was a problem hiding this comment.
Makes it easier to check if the spec is already present, find it again, naturally ensures that there is no duplication, easier to replace as well. Basically, indexing the specs by their name.
| final var dependent = createAndConfigureFrom(drs, kubernetesClient); | ||
| // check if dependent implements Deleter to record that fact | ||
| if (!hasDeleterHolder[0] && dependent instanceof Deleter) { | ||
| hasDeleterHolder[0] = true; | ||
| } | ||
| dependentsHolder.put(name, dependent); | ||
| dependentsHolder.put(drs.getName(), dependent); | ||
| }); | ||
| dependents = Collections.unmodifiableMap(dependentsHolder); |
There was a problem hiding this comment.
If we don't want to use list but an ordered Map, thus LinkedHashMap, we should have it also on the defined variable on line 57, so it's explicit on the definition, maybe also comment this. (For some reason I cannot comment on that line)
Named for me personally is better readabel / more expressive. But no strong opinion, fine also with this if the root definition is also LinkedHashMap
There was a problem hiding this comment.
The problem if we do that is that we cannot use Collections.emptyMap or Collections.unmodifiableMap anymore… Then again, since the dependents are not exposed, it's probably OK and indeed conveys the intent more clearly.
| @@ -25,13 +25,17 @@ void replaceNamedDependentResourceConfigShouldWork() { | |||
| var dependents = configuration.getDependentResources(); | |||
There was a problem hiding this comment.
This test needs to be breaked down to multiple separate test cases. It's very hard to read this way.
|
Ahh and it's my PR, will update :) |
|
Kudos, SonarCloud Quality Gate passed! |
No description provided.