Add InChainDeploymentPlannable RPC #2832

khanhtc1202 · 2021-11-24T06:40:39Z

What this PR does / why we need it:

This PR adds an RPC which be used by piped to determine whether to start planning a deployment that is triggered by the DeploymentChain model.

Which issue(s) this PR fixes:

Follow PR #2815

Does this PR introduce a user-facing change?:

NONE

pipecd-bot · 2021-11-24T06:48:01Z

Code coverage for golang is 31.79%. This pull request decreases coverage by -0.06%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/piped_api.go	PipedAPI.InChainDeploymentPlannable	`--`	`0.00%`	`+0.00%`
pkg/datastore/deploymentchainstore.go	deploymentChainStore.GetDeploymentChain	`--`	`0.00%`	`+0.00%`

nghialv · 2021-11-24T07:04:47Z

pkg/app/api/grpcapi/piped_api.go

+	prevBlock := dc.Blocks[req.Deployment.DeploymentChainBlockIndex-1]
+	plannable := true
+	for _, node := range prevBlock.Nodes {
+		dp, err := a.deploymentStore.GetDeployment(ctx, node.DeploymentRef.DeploymentId)


I think we need the deployment status in the chain model for rendering on the web so how about having that status field in the node. Then we can use it directly instead of retrieving from DB like this.

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 IsCompleted to the deploymentRef in the chain model?

In the second thought, status is required for showing in the UI, so I think I will go with your suggestion 👍

@nghialv Can I add a TODO note here and address it later since that change requires updates in other RPCs unrelated to this PR.

Addressed by e883d32

pipecd-bot · 2021-11-24T08:08:02Z

Code coverage for golang is 31.79%. This pull request decreases coverage by -0.06%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/piped_api.go	PipedAPI.InChainDeploymentPlannable	`--`	`0.00%`	`+0.00%`
pkg/datastore/deploymentchainstore.go	deploymentChainStore.GetDeploymentChain	`--`	`0.00%`	`+0.00%`

pkg/model/deployment.go

pipecd-bot · 2021-11-24T08:20:16Z

Code coverage for golang is 31.79%. This pull request decreases coverage by -0.06%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/piped_api.go	PipedAPI.InChainDeploymentPlannable	`--`	`0.00%`	`+0.00%`
pkg/datastore/deploymentchainstore.go	deploymentChainStore.GetDeploymentChain	`--`	`0.00%`	`+0.00%`
pkg/model/deployment.go	IsCompletedSuccessfullyDeployment	`--`	`0.00%`	`+0.00%`

pkg/app/api/service/pipedservice/service.proto

ono-max · 2021-11-24T08:28:37Z

/lgtm

Co-authored-by: Naoto Ono <[email protected]>

pipecd-bot · 2021-11-24T08:36:46Z

Code coverage for golang is 31.79%. This pull request decreases coverage by -0.06%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/piped_api.go	PipedAPI.InChainDeploymentPlannable	`--`	`0.00%`	`+0.00%`
pkg/datastore/deploymentchainstore.go	deploymentChainStore.GetDeploymentChain	`--`	`0.00%`	`+0.00%`
pkg/model/deployment.go	IsCompletedSuccessfullyDeployment	`--`	`0.00%`	`+0.00%`

nakabonne · 2021-11-24T09:04:47Z

pkg/app/api/grpcapi/piped_api.go

+		return nil, status.Error(codes.InvalidArgument, "invalid deployment with chain block index provided")
+	}
+
+	prevBlock := dc.Blocks[req.Deployment.DeploymentChainBlockIndex-1]


Since DeploymentChainBlockIndex is int32 which is signed integer whose range is -2147483648 to 2147483647, this is a bit likely to be panic.

How about making DeploymentChainBlockIndex an unsigned integer, or adding a simple validation?

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 👍

Fine to me 👍

nakabonne · 2021-11-24T09:09:19Z

pkg/model/deployment.go

 }

+// IsCompletedSuccessfullyDeployment checks whether the deployment is successfully addressed.
+func IsCompletedSuccessfullyDeployment(status DeploymentStatus) bool {


nit: I feel like this is more natural but it's on you

Suggested change

func IsCompletedSuccessfullyDeployment(status DeploymentStatus) bool {

func IsSuccessfullyCompletedDeployment(status DeploymentStatus) bool {

okay, lets me adopt your suggestion 🙆

addressed by 640e172

…cd/pipe into add-check-deployment-plannable-rpc

pipecd-bot · 2021-11-24T09:22:58Z

The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use /todo skip command.

Details

1. Consider add deployment status to the deployment ref in the deployment chain model

https://github.com/pipe-cd/pipe/blob/ac17a844b3a6ddce09b594da66440f5acac6e836/pkg/app/api/grpcapi/piped_api.go#L1141-L1144

This was created by todo plugin since "TODO:" was found in `ac17a84` when #2832 was merged. cc: @khanhtc1202.

pipecd-bot · 2021-11-24T09:30:46Z

Code coverage for golang is 31.79%. This pull request decreases coverage by -0.06%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/piped_api.go	PipedAPI.InChainDeploymentPlannable	`--`	`0.00%`	`+0.00%`
pkg/datastore/deploymentchainstore.go	deploymentChainStore.GetDeploymentChain	`--`	`0.00%`	`+0.00%`
pkg/model/deployment.go	IsSuccessfullyCompletedDeployment	`--`	`0.00%`	`+0.00%`

nakabonne · 2021-11-24T09:33:07Z

Keep it up!
/lgtm

nghialv · 2021-11-24T10:04:33Z

/approve

pipecd-bot · 2021-11-24T10:04:36Z

This pull request is APPROVED by nghialv.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

**What this PR does / why we need it**: - Remove unused ChainBlockIndex field from DeploymentChain model's block - Change type of DeploymentChainBlockIndex from `int32` to `uinit32` **Which issue(s) this PR fixes**: As discussed in #2832 (comment) **Does this PR introduce a user-facing change?**:  ```release-note NONE ``` This PR was merged by Kapetanios.

**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?**:  ```release-note NONE ``` This PR was merged by Kapetanios.

Add InChainDeploymentPlannable RPC

5edb751

pipecd-bot added release-note-none size/M area/api labels Nov 24, 2021

pipecd-bot requested review from nakabonne and nghialv November 24, 2021 06:40

nghialv reviewed Nov 24, 2021

View reviewed changes

Add TODO

e883d32

khanhtc1202 added 2 commits November 24, 2021 17:10

Move checking condition to model

912f6eb

Remove change

daac3be

khanhtc1202 commented Nov 24, 2021

View reviewed changes

pkg/model/deployment.go Outdated Show resolved Hide resolved

Update pkg/model/deployment.go

615d180

ono-max reviewed Nov 24, 2021

View reviewed changes

pkg/app/api/service/pipedservice/service.proto Outdated Show resolved Hide resolved

pipecd-bot added the lgtm label Nov 24, 2021

Update pkg/app/api/service/pipedservice/service.proto

7dacff3

Co-authored-by: Naoto Ono <[email protected]>

pipecd-bot removed the lgtm label Nov 24, 2021

nakabonne reviewed Nov 24, 2021

View reviewed changes

khanhtc1202 added 2 commits November 24, 2021 18:21

Rename func

640e172

Merge branch 'add-check-deployment-plannable-rpc' of github.com:pipe-…

ac17a84

…cd/pipe into add-check-deployment-plannable-rpc

pipecd-bot added the lgtm label Nov 24, 2021

pipecd-bot added the approved label Nov 24, 2021

khanhtc1202 mentioned this pull request Nov 24, 2021

Change DeploymentChainBlockIndex type #2835

Merged

pipecd-bot merged commit 0ab6cf0 into master Nov 24, 2021

pipecd-bot deleted the add-check-deployment-plannable-rpc branch November 24, 2021 10:13

pipecd-bot mentioned this pull request Nov 24, 2021

Consider add deployment status to the deployment ref in the deployment chain model #2836

Closed

khanhtc1202 mentioned this pull request Nov 29, 2021

Fix wrong condition on determine deployment plannable #2860

Merged

This was referenced Dec 1, 2021

Update doc for deployment triggering #2867

Merged

Temporarily disable appconfig reporter in Piped #2868

Merged

	func IsCompletedSuccessfullyDeployment(status DeploymentStatus) bool {
	func IsSuccessfullyCompletedDeployment(status DeploymentStatus) bool {

Add InChainDeploymentPlannable RPC #2832

Add InChainDeploymentPlannable RPC #2832

Conversation

khanhtc1202 commented Nov 24, 2021

Uh oh!

pipecd-bot commented Nov 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khanhtc1202 Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Nov 24, 2021

Uh oh!

Uh oh!

pipecd-bot commented Nov 24, 2021

Uh oh!

Uh oh!

ono-max commented Nov 24, 2021

Uh oh!

pipecd-bot commented Nov 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Nov 24, 2021

1. Consider add deployment status to the deployment ref in the deployment chain model

This was created by todo plugin since "TODO:" was found in ac17a84 when #2832 was merged. cc: @khanhtc1202.

Uh oh!

pipecd-bot commented Nov 24, 2021

Uh oh!

nakabonne commented Nov 24, 2021

Uh oh!

nghialv commented Nov 24, 2021

Uh oh!

pipecd-bot commented Nov 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

khanhtc1202 Nov 24, 2021 •

edited

Loading

This was created by todo plugin since "TODO:" was found in `ac17a84` when #2832 was merged. cc: @khanhtc1202.