Fix wrong condition on determine deployment plannable #2860
Conversation
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. Consider add deployment status to the deployment ref in the deployment chain modelThis was created by todo plugin since "TODO:" was found in 73aec2e when #2860 was merged. cc: @khanhtc1202. |
|
/todo skip |
|
Code coverage for golang is
|
| deploymentIds = append(deploymentIds, node.DeploymentRef.DeploymentId) | ||
| } | ||
|
|
||
| // TODO: Consider add deployment status to the deployment ref in the deployment chain model | ||
| // instead of fetching deployment model here. | ||
| blockDeployments, _, err := a.deploymentStore.ListDeployments(ctx, datastore.ListOptions{ | ||
| Filters: []datastore.ListFilter{ | ||
| { | ||
| Field: "Id", | ||
| Operator: datastore.OperatorIn, | ||
| Value: deploymentIds, | ||
| }, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return nil, status.Error(codes.Internal, "unable to process previous block nodes in deployment chain") | ||
| } | ||
|
|
||
| plannable := true | ||
| for _, dp := range blockDeployments { | ||
| if !model.IsSuccessfullyCompletedDeployment(dp.Status) { |
There was a problem hiding this comment.
Could you tell me what was wrong with determining logic?
Or we just changed from using Get to List with the In operator.
There was a problem hiding this comment.
The condition is flipped, previously it was
if model.IsSuccessfullyCompletedDeployment(dp.Status) {There was a problem hiding this comment.
And as you said, I changed the implementation to use In operator to reduce database read, but the TODO note has remained. I will address the issue of adding deployment status to the node.DeploymentRef later, currently just want to focus on the deployment chain flow works 🙏
|
/lgtm |
|
Good catch |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Follow PR #2832
Does this PR introduce a user-facing change?: