-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 Controller Runtime version to 0.6 #6568
Bump Controller Runtime version to 0.6 #6568
Conversation
@umangachapagain , I have updated the dependencies' versions to v0.19.3 . Please take a look... |
3456b4d
to
d4a6021
Compare
@aruniiird Some more code updates are needed, see the errors in the CI. |
d4a6021
to
c703e48
Compare
Addressed all other issues, but now I have hit the following
|
You might need to make a PR against https://github.com/kube-object-storage/lib-bucket-provisioner |
Let's discuss today, in the meantime, I have raised kube-object-storage/lib-bucket-provisioner#197 |
@aruniiird my PR on lib-bucket-provisioner has been merged you can rebase your work on top of it. |
c703e48
to
0ee8420
Compare
8fefcf6
to
4964105
Compare
All the |
All the lint / vet errors are handled... |
e7f155d
to
732d29d
Compare
@@ -46,23 +48,27 @@ func New(context *clusterd.Context) (Attachment, error) { | |||
|
|||
// Get queries the Volume CRD from Kubernetes | |||
func (c *crd) Get(namespace, name string) (*rookalpha.Volume, error) { | |||
return c.context.RookClientset.RookV1alpha2().Volumes(namespace).Get(name, metav1.GetOptions{}) | |||
ctx := context.TODO() |
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.
you don't need ctx
just pass context.TODO()
directly.
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.
Yup done...
Initial idea was to make changes to the attachment.Attachment
interface itself (to match it the same with the latest api changes), but later refrained from it.
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.
What's done I still see ctx := context.TODO()
?
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.
Corrected... I missed other lines...
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.
I still don't see anything
@@ -120,13 +121,17 @@ func (o *Operator) Run() error { | |||
} | |||
} | |||
|
|||
// creating a context | |||
stopContext, stopFunc := context.WithCancel(context.Background()) |
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.
please rename stopFunc
to contextCancel
it's clearer and stopContext
just ctx
, it's more common.
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.
also, how about using defer
instead of calling stopFunc()
everywhere?
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.
Good point... Yup made the change...
e738761
to
d6370a6
Compare
This pull request has merge conflicts that must be resolved before it can be merged. @aruniiird please rebase it. https://rook.io/docs/rook/master/development-flow.html#updating-your-fork |
d6370a6
to
20f500d
Compare
Updating the dependencies' versions to match with the newer Operator SDK version v1.x Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>
Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>
20f500d
to
e3b9241
Compare
Fetched latest lib-bucket-provisioner changes as well. Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>
Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>
ddb1419
to
25cf3f4
Compare
Signed-off-by: Arun Kumar Mohan <amohan@redhat.com>
25cf3f4
to
f5fc08b
Compare
We must add the finalizer right after the object creation otherwise the seerver will later return an error on update that the object has been modified. Indeed, it has been by the task that updates the status when the object is first created. Signed-off-by: Sébastien Han <seb@redhat.com>
We don't need to lint go.sum since it's auto generated. Signed-off-by: Sébastien Han <seb@redhat.com>
How close is this to being merged? I'm building multiple fixes for #6650 on top of this for now. |
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.
Would it make sense to merge first 4 commits into 1? It's all part of single deps upgrade.
Also, let's not mention SDK here. This is more about supporting K8s 1.19 and Controller Runtime 0.6.z.
It will be merged today, I want to manually test it. |
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.
Successfully tested the change, no side effects observed.
Bump Controller Runtime version to 0.6 (bp #6568)
Signed-off-by: Arun Kumar Mohan amohan@redhat.com
Description of your changes:
Which issue is resolved by this Pull Request:
Resolves #
Checklist:
make codegen
) has been run to update object specifications, if necessary.