-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: Support set image operation for sidecarsets #72
Conversation
Welcome @chengleqi! It looks like this is your first PR to openkruise/kruise-tools 🎉 |
Signed-off-by: chengleqi <chengleqi5g@hotmail.com>
b0a8b57
to
b372841
Compare
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.
/LGTM 🚀
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hantmac The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold sorry @hantmac I found a typo. |
}, | ||
}, | ||
groupVersion: corev1.SchemeGroupVersion, | ||
path: "/namespaces/test/sidecarsets/nginx", |
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.
since sidecarset is a cluster scoped resource, i doubt whether the path test is valid here
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.
Because the SidecarSet is a cluster-wide resource, it is defined across all namespaces and can be operated on by all namespaces, even the namespaces does not exist.
This can work
# dummy-ns do not existed
[root@zone-a code]# kubectl get sidecarset -n dummy-ns
NAME MATCHED UPDATED READY AGE
test-sidecarset 1 1 1 28m
This also can work
# dummy-ns do not existed
kubectl kruise set image sidecarsets/test-sidecarset sidecar1=debian:11 -n dummy-ns
# output sidecarset.apps.kruise.io/test-sidecarset image updated
So I think the namespace is not important for cluster-scoped resources, and the only identifier that can distinguish cluster-scoped resources is their name.
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.
/lgtm
Result
User can use
kubectl kruise set image sidecarsets/test-sidecarset sidecar1=debian:11
Fix Issue
Fix: openkruise/kruise#1106
Change
kruiseappsv1alpha1.SidecarSet
branch.kruiseappsv1alpha1.SidecarSet
type then usesetSideCarImage
function which is similar withsetImage
.