Owls88697 stop namespaces that no longer managed by operator when change to dedicated strategy #2345
Conversation
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
rjeberhard
approved these changes
Apr 28, 2021
russgold
reviewed
Apr 30, 2021
Contributor
russgold
left a comment
russgold
left a comment
There was a problem hiding this comment.
Generally looks good; I have left a style recommendation.
| defineSelectionStrategy(SelectionStrategy.List); | ||
| String namespaceString = "NS1"; | ||
| HelmAccessStub.defineVariable(HelmAccess.OPERATOR_DOMAIN_NAMESPACES, namespaceString); | ||
| createNamespaces(4); |
Contributor
There was a problem hiding this comment.
I have found that organizing tests can help readability:
- general setup (what the world looks like before the operation)
- the action to test
- the assertion.
You can do that with judicious use of spaces. Here, lines 845-850 establish your starting point, and 852-854 are the action, so I would remove the blank lines in between them, and reserve those as separators only between the three sections.
If those are complex, you might consider extract them to their own methods, especially if you do the same setup in multiple tests.
russgold
reviewed
Apr 30, 2021
| public void withNamespaceList_changeToDedicated_onCreateReadNamespaces_nsWatchStartedEventCreatedInOpNS() { | ||
| setupDomainWithNamespaceListStrategy("NS1"); | ||
| runCreateReadNamespacesStep(); | ||
| defineSelectionStrategy(SelectionStrategy.Dedicated); |
Contributor
There was a problem hiding this comment.
Here you are changing to a new strategy, so line 859 begins the 'action' part of the test and should be separated by a blank line from the 'setup'
russgold
approved these changes
Apr 30, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
For List, RegExp and LabelSelector domain selection strategy, the operator cleans up the namespaces that it does not managed any more in Namespaces.NamespaceListAfterStep. The cleanup is missed for the dedicated case because the operator does not go through the steps to list the existing namespaces. The fix is to add the Namespaces.NamespaceListAfterStep to the dedicated case and customize the getFoundDomainNamespaces to return the operator namespace.
The PR also makes the operator perform more necessary clean upon stopping managing a namespace or when a domain is deleted.
Unit test cases are added to cover the scenarios associated with the code changes. Some existing unit test cases where a particular domain namespaces selection strategy is tested are modified to run CreateReadNamespacesStep instead of readExsitingNamespaces because the latter does not exercise the strategy specific code path.
The test case in ItKuerbenetesEvents.java for dedicated strategy that used to fail because of this issue is turned on.
https://build.weblogick8s.org:8443/job/weblogic-kubernetes-operator-kind-new/4870/