SidecarSet inject sidecar container to all Pods in some namespaces #1369
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: pingjiang <xiangpingjiang1998@gmail.com>
xiangpingjiang
force-pushed
the
SidecarSet_NameSpace
branch
from
05ffe4a
to
93326ca
Compare
| @@ -1244,6 +1244,32 @@ func TestPodMatchedSidecarSet(t *testing.T) { | |||
| }, | |||
| expect: false, | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
You need modidy the sidecarSet webhook and controller.
And you must add e2e for this scenario.
There was a problem hiding this comment.
Done, please review my latest pr, thanks
Signed-off-by: pingjiang <xiangpingjiang1998@gmail.com>
xiangpingjiang
force-pushed
the
SidecarSet_NameSpace
branch
from
776fc18
to
c7959cc
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1369 +/- ##
==========================================
- Coverage 48.25% 48.21% -0.04%
==========================================
Files 152 152
Lines 21469 21476 +7
==========================================
- Hits 10359 10354 -5
- Misses 9973 9981 +8
- Partials 1137 1141 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -103,6 +103,10 @@ func PodMatchedSidecarSet(c client.Client, pod *corev1.Pod, sidecarSet *appsv1al | |||
| if err != nil { | |||
| return false, err | |||
| } | |||
| // if no selector set, return true | |||
| if selector.String() == "" { | |||
There was a problem hiding this comment.
selector.Empty() return false when no selector set, so I turned to selector.String() == ""
| // if no selector set, return true | ||
| if selector.String() == "" { | ||
| return true, nil | ||
| } | ||
|
|
||
| if !selector.Empty() && selector.Matches(labels.Set(pod.Labels)) { |
| @@ -310,6 +310,9 @@ func (p *Processor) getMatchingPods(s *appsv1alpha1.SidecarSet) ([]*corev1.Pod, | |||
| func (p *Processor) getSelectedPods(namespaces sets.String, selector labels.Selector) (relatedPods []*corev1.Pod, err error) { | |||
| // DisableDeepCopy:true, indicates must be deep copy before update pod objection | |||
| listOpts := &client.ListOptions{LabelSelector: selector} | |||
| if selector.String() == "" { | |||
There was a problem hiding this comment.
Remove the If, and i think it will be ok.
There was a problem hiding this comment.
It won't work,
Because if we remove
if selector.String() == "" {
listOpts = &client.ListOptions{}
}
when sidecarSet.Spec.Selector = nil , p.Client.List(context.TODO(), allPods, listOpts, utilclient.DisableDeepCopy) get always allPods.Items = 0.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
/hold |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Ⅰ. Describe what this PR does
Ⅱ. Does this pull request fix one issue?
fixes #1368
Ⅲ. Describe how to verify it
Ⅳ. Special notes for reviews