-
Notifications
You must be signed in to change notification settings - Fork 101
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
Bump operator SDK to v1.5.0 #419
Conversation
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.
A little bit concerned with controller-runtime version bump. Was the stuff e2e tested (outside of terratest) ?
@@ -53,7 +53,7 @@ type GslbReconciler struct { | |||
} | |||
|
|||
const ( | |||
gslbFinalizer = "finalizer.k8gb.absa.oss" | |||
gslbFinalizer = "k8gb.absa.oss/finalizer" |
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.
This might not work well with existing gslbs having "finalizer.k8gb.absa.oss" finalizer set. Imo we need to use an array of valid finalizers here (["finalizer.k8gb.absa.oss" , "k8gb.absa.oss/finalizer"]
) in order to support smooth upgrade for set/get finalizer logic.
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.
@somaritane Conceptually agreed. But how the removal logic will work in that case? Just in case, in order to delete resource we need to remove all finalizers from the object
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.
@ytsarev In case of finalizers deletion, we can remove all finalizers that correspond to that list: ["finalizer.k8gb.absa.oss" , "k8gb.absa.oss/finalizer"]
.
In case of finalizer addition, we add only new version of finalizer: "k8gb.absa.oss/finalizer"
This way all gslbs will be eventually migrated to have only new finalizers.
But if we only replace "finalizer.k8gb.absa.oss" with "k8gb.absa.oss/finalizer" in the code, we end up with "finalizer.k8gb.absa.oss" not being deleted, and gslb deletion stuck.
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.
Sounds like a plan
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.
amending code but how do we say that new instance is "k8gb.absa.oss/finalizer"
and not an array ?
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.
amended,
see: the loop removing finalizers
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.
amended, test coverage for finalizers
5e3225f
to
e3cf2bf
Compare
50764ed
to
5bde05e
Compare
close #376 - v1.4.0 Change operator's finalizer names (finalizer.cache.example.com should be changed to cache.example.com/finalizer) - v1.4.0 directory argument in docker-build was moved to the last position to align with podman's expectations, making substitution cleaner. - v1.5.0 `PROJECT` config version `3-alpha1` must be upgraded to `"3"` - v1.5.0 Upgrade controller-runtime to v0.7.2 controller runtime. The package changed and controller must implement Reconciler interface. To be able to build project I had to implement interface (by adding context to Reconcile function) and use `client.Object` in handlers - test coverage Signed-off-by: kuritka <[email protected]>
5bde05e
to
3840d44
Compare
@ytsarev , Upgrade test is successful + I also made local playground test. |
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.
👍
Cool stuff, let's merge it |
close #376
finalizer.cache.example.com
should be changed tocache.example.com/finalizer
)docker-build
was moved to the last position to align withpodman's
expectations, making substitution cleaner.PROJECT
config version3-alpha
must be upgraded to"3"
v0.7.2
controller runtime.The package changed, and controller must implement Reconciler interface.
To be able to build project, I had to implement Reconciler interface (by adding context to Reconcile function)
and use
client.Object
in handler functionslinks