create empty decision when no cluster selected #95
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haoqing0110 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 |
haoqing0110
force-pushed
the
br_empty-decision
branch
from
f1d2bf6 to
18977a0
Compare
| for index := 0; len(remainingDecisions) > 0; index++ { | ||
| // if clusterDecisions is empty, append empty slice to decisionSlices, | ||
| // and create an PlacementDecision with empty decisions in status. | ||
| for index := 0; len(remainingDecisions) > 0 || len(decisionSlices) == 0; index++ { |
There was a problem hiding this comment.
This is confusing, can we make it more explicit
haoqing0110
force-pushed
the
br_empty-decision
branch
from
18977a0 to
7eb6d01
Compare
| // so that can create an PlacementDecision with empty decisions in status. | ||
| if len(decisionSlices) == 0 { | ||
| decisionSlice := []clusterapiv1beta1.ClusterDecision{} | ||
| decisionSlices = append(decisionSlices, decisionSlice) |
There was a problem hiding this comment.
nit: decisionSlices = append(decisionSlices, []clusterapiv1beta1.ClusterDecision{})
Signed-off-by: haoqing0110 <qhao@redhat.com>
haoqing0110
force-pushed
the
br_empty-decision
branch
from
7eb6d01 to
6c92d85
Compare
| // requeue placement if requeueAfter is defined in scheduleResult | ||
| if syncCtx != nil && scheduleResult.RequeueAfter() != nil { | ||
| if syncCtx != nil && !status.IsError() && scheduleResult.RequeueAfter() != nil { |
There was a problem hiding this comment.
I think we should requeue no matter there is error or not? and should we return error finally after update status?
haoqing0110
force-pushed
the
br_empty-decision
branch
from
ae58837 to
a4afc4b
Compare
| @@ -260,7 +252,12 @@ func (c *schedulingController) syncPlacement(ctx context.Context, syncCtx factor | |||
| } | |||
|
|
|||
| // update placement status if necessary to signal no bindings | |||
| return c.updateStatus(ctx, placement, int32(len(scheduleResult.Decisions())), misconfiguredCondition, satisfiedCondition) | |||
| err = c.updateStatus(ctx, placement, int32(len(scheduleResult.Decisions())), misconfiguredCondition, satisfiedCondition) | |||
There was a problem hiding this comment.
nit
if err := xx ; err !=nil {
return err
}
| @@ -81,6 +81,14 @@ func (s *Status) AsError() error { | |||
| return errors.New(s.Message()) | |||
| } | |||
|
|
|||
| // Error returns the err when Code is "Error" or "Misconfigured". | |||
| func (s *Status) Error() error { | |||
| if s.IsError() { | |||
There was a problem hiding this comment.
hrm, we might want to refactor here. why we have both isError and asError? couldn't we just check if returned error is nil?
There was a problem hiding this comment.
It's for the case when Status is nil I think. Remove this func. And modify the NewStatus(), to ensure AsError can always return s.err when code is Error or Misconfigured.
haoqing0110
force-pushed
the
br_empty-decision
branch
4 times, most recently
from
274706a to
b33c54e
Compare
Signed-off-by: haoqing0110 <qhao@redhat.com>
haoqing0110
force-pushed
the
br_empty-decision
branch
from
b33c54e to
a4d8c6d
Compare
|
/lgtm |
#85
Signed-off-by: haoqing0110 qhao@redhat.com