Add check plannable deployment logic #2844
Conversation
|
Unabled to get changed files of pull request. Details |
|
Code coverage for golang is
|
| } | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we have any way to show the reason why a deployment has not been planned on the web?
Should we set that message by trigger or planner?
There was a problem hiding this comment.
Currently, we don't have, since the state of the deployment is not changed, the message for that keep PENDING may be redundant to me. If we want to show kind of reason why the deployment keeps pending, we may need a new RPC like UpdateDeploymentStatusReason or such. How do you think?
There was a problem hiding this comment.
I just wanted to ask to know did you have any plan for that thing yet.
Could you create a new issue to ensure that it could not be flushed out of our memory over time?
Btw, I will approve this PR since that is out of the scope of this PR.
There was a problem hiding this comment.
I see, in that case
Should we set that message by trigger or planner?
Probably not the trigger since after triggering the deployment to start (PENDING state) the trigger is out of action, and this check is plannable logic run in syncPlaner phase but before a planner starts running, so the one who submits that message should be controller itself, via kind of UpdateDeploymentStatusReason RPC (the deployment state is unchanged but update the desc?) that is in my image. Let's me think about that in detail later (will create issue for that)
| return true, nil | ||
| } | ||
| resp, err := c.apiClient.InChainDeploymentPlannable(ctx, &pipedservice.InChainDeploymentPlannableRequest{ | ||
| Deployment: d, |
There was a problem hiding this comment.
Sorry I have just found a thing.
Do you need to send the whole deployment object?
It might be a bit redundant.
There was a problem hiding this comment.
To determine should we plan that deployment, currently we need: DeploymentChainId and DeploymentChainBlockIndex, not the whole deployment object. Should we only submit those 2 fields? I'm okay with both but in the future, if we need more than that to determine if this deployment should be planed or not, we have to update RPC too.
There was a problem hiding this comment.
I see.
Let's change the RPC to use just needed fields.
|
Code coverage for golang is
|
|
Nice. |
|
Cool |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
Check plannability of the deployment before actually planning it using InChainPlannableDeployment RPC
Which issue(s) this PR fixes:
Fixes #2827
Does this PR introduce a user-facing change?: