controller code/test refactoring/additions #107
Closed
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
openshift-ci
bot
added
the
do-not-merge/work-in-progress
joelanford
force-pushed
the
refactor-plus-unit-tests
branch
from
683b79e
to
3dfea44
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
tmshort
reviewed
Feb 1, 2023
joelanford
force-pushed
the
refactor-plus-unit-tests
branch
from
3dfea44
to
5d27194
Compare
tmshort
reviewed
Feb 1, 2023
main bullet points are:
- decrease nesting in main reconcile code by setting conditions and
returning early when handling unexpected errors.
- move reconciler unit tests to a separate file from the suite. if we
add other controllers in the future, we'll likely want separate test
files for each controller.
- export the reconciler's resolver field to make it easy to swap out for
tests (also add and use a test entity source)
- remove the manager setup in the unit tests. this ensures we can test
individual runs of the Reconcile function, which is good for two
reasons:
1. no need to use `Eventually`, no polling, etc. so it speeds up the tests
2. it ensures that we fully understand the transitions that occur from
a beginning state to and end state.
- added more unit tests to increase code coverage.
- moved conditions and reasons checks to invariants that run after every
test of reconciling an existing Operator object.
- make certain bundle entity properties optional. not all bundles will
provide/require GVKs or require other packages. lookups for those
entities should return nil slices rather than an error.
- remove unnecessary RBAC rules for create/update/patch/delete Operator
objects. this leaves only get, list, watch, update status, and update
finalizers.
joelanford
force-pushed
the
refactor-plus-unit-tests
branch
from
5d27194
to
bc627b6
Compare
tmshort
reviewed
Feb 1, 2023
tmshort
reviewed
Feb 1, 2023
1. factor bundle lookup into separate function, which makes for less complex main reconcile function 2. use server-side apply to ensure bundle deployment is up-to-date 3. add more unit test scenarios
|
I added another commit, which I think improves the controller code a bit more. It also adds more unit test scenarios. I'm fairly happy with those, but they do introduce some duplication that I think could be reduced a bit, so I think its worth a follow up to focus on that. |
perdasilva
reviewed
Feb 2, 2023
| if err = (&controllers.OperatorReconciler{ | ||
| Client: mgr.GetClient(), | ||
| Scheme: mgr.GetScheme(), | ||
| Resolver: resolution.NewOperatorResolver(mgr.GetClient(), resolution.HardcodedEntitySource), |
This was referenced Feb 2, 2023
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
perdasilva
reviewed
Feb 3, 2023
| @@ -154,7 +154,7 @@ func (b *BundleEntity) loadRequiredGVKs() error { | |||
| b.mu.Lock() | |||
| defer b.mu.Unlock() | |||
| if b.requiredGVKs == nil { | |||
| requiredGVKs, err := loadFromEntity[[]GVKRequired](b.Entity, property.TypeGVKRequired) | |||
| requiredGVKs, err := loadFromEntity[[]GVKRequired](b.Entity, property.TypeGVKRequired, false) | |||
There was a problem hiding this comment.
as we discussed online, I think we should only push these booleans to constants to make it a little more readable and that's basically the cherry on top ^^
might be worth splitting the suite_test changes to a different PR. But not strictly necesssary...
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.
I noticed y'all made some great progress this week, and got the itch to stay in the loop and pitch in. This PR is a whole bunch of changes all wrapped up together, so I have no illusions about it being merged as is. I'm curious what folks think!
Main bullet points are:
objects. this leaves only get, list, watch, update status, and update
finalizers. (reduce RBAC permissions of operator-controller #116)
Eventually, no polling, etc. so it speeds up the testsEDIT: I split each of these main things out into separate PRs now that it seems like folks are generally onboard.