-
Notifications
You must be signed in to change notification settings - Fork 208
Add InChainDeploymentPlannable RPC #2832
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
Changes from 4 commits
5edb751
e883d32
912f6eb
daac3be
615d180
7dacff3
640e172
ac17a84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1105,6 +1105,56 @@ func (a *PipedAPI) CreateDeploymentChain(ctx context.Context, req *pipedservice. | |
| return &pipedservice.CreateDeploymentChainResponse{}, nil | ||
| } | ||
|
|
||
| // InChainDeploymentPlannable hecks the completion and status of the previous block in the deployment chain. | ||
| // An in chain deployment is treated as plannable in case: | ||
| // - It's the first deployment of its deployment chain. | ||
| // - All deployments of its previous block in chain are at DEPLOYMENT_SUCCESS state. | ||
| func (a *PipedAPI) InChainDeploymentPlannable(ctx context.Context, req *pipedservice.InChainDeploymentPlannableRequest) (*pipedservice.InChainDeploymentPlannableResponse, error) { | ||
| _, pipedID, _, err := rpcauth.ExtractPipedToken(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if err := a.validateDeploymentBelongsToPiped(ctx, req.Deployment.Id, pipedID); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| dc, err := a.deploymentChainStore.GetDeploymentChain(ctx, req.Deployment.DeploymentChainId) | ||
| if err != nil { | ||
| return nil, status.Error(codes.InvalidArgument, "unable to find the deployment chain which this deployment belongs to") | ||
| } | ||
|
|
||
| // Deployment of blocks[0] in the chain means it's the first deployment of the chain; | ||
| // hence it should be processed without any lock. | ||
| if req.Deployment.DeploymentChainBlockIndex == 0 { | ||
| return &pipedservice.InChainDeploymentPlannableResponse{ | ||
| Plannable: true, | ||
| }, nil | ||
| } | ||
|
|
||
| if req.Deployment.DeploymentChainBlockIndex >= int32(len(dc.Blocks)) { | ||
| return nil, status.Error(codes.InvalidArgument, "invalid deployment with chain block index provided") | ||
| } | ||
|
|
||
| prevBlock := dc.Blocks[req.Deployment.DeploymentChainBlockIndex-1] | ||
| plannable := true | ||
| for _, node := range prevBlock.Nodes { | ||
| // TODO: Consider add deployment status to the deployment ref in the deployment chain model | ||
| // instead of fetching deployment model here. | ||
| dp, err := a.deploymentStore.GetDeployment(ctx, node.DeploymentRef.DeploymentId) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need the deployment status in the chain model for rendering on the web so how about having that
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case, when piped submits deployment status (planed, running, etc) we also need to update its status in deployment chain model too, that's quite annoying. How do you think about adding just
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the second thought,
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nghialv Can I add a TODO note here and address it later since that change requires updates in other RPCs unrelated to this PR.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed by e883d32 |
||
| if err != nil { | ||
| return nil, status.Error(codes.Internal, "unable to process previous block nodes in deployment chain") | ||
| } | ||
| if model.IsCompletedSuccessfullyDeployment(dp.Status) { | ||
| plannable = false | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return &pipedservice.InChainDeploymentPlannableResponse{ | ||
| Plannable: plannable, | ||
| }, nil | ||
| } | ||
|
|
||
| // validateAppBelongsToPiped checks if the given application belongs to the given piped. | ||
| // It gives back an error unless the application belongs to the piped. | ||
| func (a *PipedAPI) validateAppBelongsToPiped(ctx context.Context, appID, pipedID string) error { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -48,6 +48,11 @@ func IsCompletedDeployment(status DeploymentStatus) bool { | |||||
| return false | ||||||
| } | ||||||
|
|
||||||
| // IsCompletedSuccessfullyDeployment checks whether the deployment is at a successfully addressed. | ||||||
khanhtc1202 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| func IsCompletedSuccessfullyDeployment(status DeploymentStatus) bool { | ||||||
|
||||||
| func IsCompletedSuccessfullyDeployment(status DeploymentStatus) bool { | |
| func IsSuccessfullyCompletedDeployment(status DeploymentStatus) bool { |
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.
okay, lets me adopt your suggestion 🙆
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.
addressed by 640e172
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
DeploymentChainBlockIndexisint32which is signed integer whose range is -2147483648 to 2147483647, this is a bit likely to be panic.How about making
DeploymentChainBlockIndexan unsigned integer, or adding a simple validation?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.
Nice catch, golang does not support negative indexes for slide, right? In that case, lets me address it in a separated PR since other place that not related to this change need to be updated too 👍
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.
Fine to me 👍