-
Notifications
You must be signed in to change notification settings - Fork 220
fix: ordered managed dependents #1120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
797642a
fix: ordered dependents
csviri 3d43b66
fix: add integration test
csviri c01c382
fix: format
csviri b658a8c
fix: IT temp assert changes
csviri 5d107dc
fix: simplify dependents overriding
metacosm 79fe952
fix: remove duplicated output
metacosm 25c23df
fix: only watch current namespace to avoid parasite ConfigMap events
metacosm ce0d381
fix: NamedDependentResource really isn't needed right now
metacosm b776a54
refactor: make dependents LinkedHashMap explicitly
metacosm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,13 +25,17 @@ void replaceNamedDependentResourceConfigShouldWork() { | |
var dependents = configuration.getDependentResources(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test needs to be breaked down to multiple separate test cases. It's very hard to read this way. |
||
assertFalse(dependents.isEmpty()); | ||
assertEquals(1, dependents.size()); | ||
|
||
final var dependentResourceName = DependentResource.defaultNameFor(ReadOnlyDependent.class); | ||
assertTrue(dependents.containsKey(dependentResourceName)); | ||
var dependentSpec = dependents.get(dependentResourceName); | ||
assertTrue(dependents.stream().anyMatch(dr -> dr.getName().equals(dependentResourceName))); | ||
|
||
var dependentSpec = dependents.stream().filter(dr -> dr.getName().equals(dependentResourceName)) | ||
.findFirst().get(); | ||
assertEquals(ReadOnlyDependent.class, dependentSpec.getDependentResourceClass()); | ||
var maybeConfig = dependentSpec.getDependentResourceConfiguration(); | ||
assertTrue(maybeConfig.isPresent()); | ||
assertTrue(maybeConfig.get() instanceof KubernetesDependentResourceConfig); | ||
|
||
var config = (KubernetesDependentResourceConfig) maybeConfig.orElseThrow(); | ||
// check that the DependentResource inherits the controller's configuration if applicable | ||
assertEquals(1, config.namespaces().length); | ||
|
@@ -47,7 +51,8 @@ void replaceNamedDependentResourceConfigShouldWork() { | |
new KubernetesDependentResourceConfig(new String[] {overriddenNS}, labelSelector)) | ||
.build(); | ||
dependents = overridden.getDependentResources(); | ||
dependentSpec = dependents.get(dependentResourceName); | ||
dependentSpec = dependents.stream().filter(dr -> dr.getName().equals(dependentResourceName)) | ||
.findFirst().get(); | ||
config = (KubernetesDependentResourceConfig) dependentSpec.getDependentResourceConfiguration() | ||
.orElseThrow(); | ||
assertEquals(1, config.namespaces().length); | ||
|
49 changes: 49 additions & 0 deletions
49
operator-framework/src/test/java/io/javaoperatorsdk/operator/OrderedManagedDependentIT.java
This file contains hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
package io.javaoperatorsdk.operator; | ||
|
||
import java.time.Duration; | ||
|
||
import org.junit.jupiter.api.Test; | ||
import org.junit.jupiter.api.extension.RegisterExtension; | ||
|
||
import io.fabric8.kubernetes.api.model.ObjectMeta; | ||
import io.javaoperatorsdk.operator.junit.OperatorExtension; | ||
import io.javaoperatorsdk.operator.sample.orderedmanageddependent.ConfigMapDependentResource1; | ||
import io.javaoperatorsdk.operator.sample.orderedmanageddependent.ConfigMapDependentResource2; | ||
import io.javaoperatorsdk.operator.sample.orderedmanageddependent.OrderedManagedDependentCustomResource; | ||
import io.javaoperatorsdk.operator.sample.orderedmanageddependent.OrderedManagedDependentTestReconciler; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.awaitility.Awaitility.await; | ||
|
||
class OrderedManagedDependentIT { | ||
|
||
@RegisterExtension | ||
OperatorExtension operator = | ||
OperatorExtension.builder().withReconciler(new OrderedManagedDependentTestReconciler()) | ||
.build(); | ||
|
||
@Test | ||
void managedDependentsAreReconciledInOrder() { | ||
operator.create(OrderedManagedDependentCustomResource.class, createTestResource()); | ||
|
||
await().atMost(Duration.ofSeconds(5)) | ||
.until(() -> ((OrderedManagedDependentTestReconciler) operator.getFirstReconciler()) | ||
.getNumberOfExecutions() >= 1); | ||
// todo change to more precise values when event filtering is fixed | ||
// assertThat(OrderedManagedDependentTestReconciler.dependentExecution).hasSize(4); | ||
assertThat(OrderedManagedDependentTestReconciler.dependentExecution.get(0)) | ||
.isEqualTo(ConfigMapDependentResource1.class); | ||
assertThat(OrderedManagedDependentTestReconciler.dependentExecution.get(1)) | ||
.isEqualTo(ConfigMapDependentResource2.class); | ||
|
||
} | ||
|
||
|
||
private OrderedManagedDependentCustomResource createTestResource() { | ||
OrderedManagedDependentCustomResource cr = new OrderedManagedDependentCustomResource(); | ||
cr.setMetadata(new ObjectMeta()); | ||
cr.getMetadata().setName("test"); | ||
return cr; | ||
} | ||
|
||
} |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.