Allow deploy source to be nil considering the first deploy #5766

ffjlabo · 2025-04-21T13:55:53Z

What this PR does:

Fix to allow deploy target to be nil considering the first deploy.

Why we need it:

When I register an app with plugin-architected pipd, the first deployment fails because the SDK does not consider the running deploy source to be nil.

FYI, this is the error msg I encountered.

failed to handle an unary gRPC request: /grpc.plugin.deploymentapi.v1alpha1.DeploymentService/ExecuteStage{"error": "rpc error: code = Internal desc = failed to create running deployment source: failed to decode application config: unsupported version: ", "duration": 3.26620525}
github.com/pipe-cd/pipecd/pkg/rpc.WithLogUnaryInterceptor.func1.LogUnaryServerInterceptor.1
        /Users/s14218/oss/pipe-cd/pipecd/pkg/rpc/log_interceptor.go:37
github.com/pipe-cd/pipecd/pkg/rpc.(*Server).init.ChainUnaryServerInterceptors.func2.1.1
        /Users/s14218/oss/pipe-cd/pipecd/pkg/rpc/chain_interceptor.go:30
github.com/pipe-cd/pipecd/pkg/rpc.(*Server).init.ChainUnaryServerInterceptors.func2
        /Users/s14218/oss/pipe-cd/pipecd/pkg/rpc/chain_interceptor.go:37
github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1/deployment._DeploymentService_ExecuteStage_Handler
        /Users/s14218/oss/pipe-cd/pipecd/pkg/plugin/api/v1alpha1/deployment/api_grpc.pb.go:260
google.golang.org/grpc.(*Server).processUnaryRPC
        /Users/s14218/go/pkg/mod/google.golang.org/[email protected]/server.go:1379
google.golang.org/grpc.(*Server).handleStream
        /Users/s14218/go/pkg/mod/google.golang.org/[email protected]/server.go:1790
google.golang.org/grpc.(*Server).serveStreams.func2.1
        /Users/s14218/go/pkg/mod/google.golang.org/[email protected]/server.go:1029

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

How are users affected by this change:
Is this breaking change:
How to migrate (if breaking change):

codecov · 2025-04-21T13:59:37Z

Codecov Report

Attention: Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 27.08%. Comparing base (1c0920b) to head (60708ca).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/plugin/sdk/deployment.go	50.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5766      +/-   ##
==========================================
+ Coverage   27.06%   27.08%   +0.01%     
==========================================
  Files         504      504              
  Lines       53178    53184       +6     
==========================================
+ Hits        14395    14407      +12     
+ Misses      37709    37703       -6     
  Partials     1074     1074

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Yoshiki Fujikane <[email protected]>

t-kikuc

almost LGTM

t-kikuc · 2025-04-22T01:39:42Z

pkg/app/pipedv1/deploysource/deploysource.go

@@ -38,6 +38,10 @@ type DeploySource struct {
 }

 func (d *DeploySource) ToPluginDeploySource() *common.DeploymentSource {
+	if d == nil {


I think this will go panic, right?

@t-kikuc
This is not go panic.
This is to avoid panic for nil pointer reference.

Referencing a nil value will result in an panic such as d.AppDir

i got it, i understood golang

If we add this check here, other toXXX converter methods could be required as well 🤔 So I think it could be better to emit that "object" here if it is not nil, don't check nil here (or we have to do that for all other methods). And my suggestion is to update this newDeploymentSource constructor (ref: https://github.com/pipe-cd/pipecd/blob/master/pkg/plugin/sdk/deployment_source.go#L39-L50), since this error looks like: call to constructor to create object but get nil without error, to me. wdyt? @ffjlabo

@khanhtc1202
According to the specifications, there are cases where DeploySource is nil, so I personally don't mind if it's included in ToXX, but what do you think?
I don't think it's necessary to check for nil in all To methods.

As discussed with @khanhtc1202, we
noticed that it might be easy to understand to check nil outside ToPluginDeploySource.
I'd be happy to hear other opinions. WDYT? @t-kikuc @Warashi

The fix↓
60708ca

I agree with you. I think it's easy to understand and enough to check whether the variable is nil outside of the ToPluginDeploySource.

i agree, it seems easier to check outside of the method.

Thank you for checking!
I'll merge the fixed one🚀

Signed-off-by: Yoshiki Fujikane <[email protected]>

khanhtc1202

LGTM 👍

t-kikuc

Good!

Warashi

LGTM 👍🏻

github-actions bot added area/go area/pipedv1 labels Apr 21, 2025

Allow deploy source to be nil considering the first deploy

0e33fa7

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo force-pushed the fix-to-avoid-failing-the-first-deployment branch from 193d5d7 to 0e33fa7 Compare April 21, 2025 23:23

ffjlabo marked this pull request as ready for review April 21, 2025 23:30

ffjlabo requested review from khanhtc1202, t-kikuc and Warashi as code owners April 21, 2025 23:30

ffjlabo enabled auto-merge (squash) April 21, 2025 23:30

t-kikuc reviewed Apr 22, 2025

View reviewed changes

ffjlabo requested a review from t-kikuc April 22, 2025 02:24

t-kikuc previously approved these changes Apr 22, 2025

View reviewed changes

Fix for readability

60708ca

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo dismissed t-kikuc’s stale review via 60708ca April 22, 2025 05:53

khanhtc1202 approved these changes Apr 22, 2025

View reviewed changes

t-kikuc disabled auto-merge April 22, 2025 06:06

t-kikuc approved these changes Apr 22, 2025

View reviewed changes

Warashi approved these changes Apr 22, 2025

View reviewed changes

ffjlabo merged commit 9244032 into master Apr 22, 2025
19 checks passed

ffjlabo deleted the fix-to-avoid-failing-the-first-deployment branch April 22, 2025 06:19

github-actions bot mentioned this pull request May 15, 2025

Release v0.51.3 #5821

Merged

github-actions bot mentioned this pull request May 28, 2025

[bot] Release v0.52.0 #5888

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow deploy source to be nil considering the first deploy #5766

Allow deploy source to be nil considering the first deploy #5766

Uh oh!

ffjlabo commented Apr 21, 2025

Uh oh!

codecov bot commented Apr 21, 2025 •

edited

Loading

Uh oh!

t-kikuc left a comment

Uh oh!

t-kikuc Apr 22, 2025

Uh oh!

ffjlabo Apr 22, 2025

Uh oh!

t-kikuc Apr 22, 2025

Uh oh!

khanhtc1202 Apr 22, 2025

Uh oh!

ffjlabo Apr 22, 2025

Uh oh!

ffjlabo Apr 22, 2025

Uh oh!

Warashi Apr 22, 2025

Uh oh!

t-kikuc Apr 22, 2025

Uh oh!

ffjlabo Apr 22, 2025

Uh oh!

khanhtc1202 left a comment

Uh oh!

t-kikuc left a comment

Uh oh!

Warashi left a comment

Uh oh!

Uh oh!

Uh oh!

Allow deploy source to be nil considering the first deploy #5766

Allow deploy source to be nil considering the first deploy #5766

Uh oh!

Conversation

ffjlabo commented Apr 21, 2025

Uh oh!

codecov bot commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

t-kikuc left a comment

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!

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!

khanhtc1202 left a comment

Choose a reason for hiding this comment

Uh oh!

t-kikuc left a comment

Choose a reason for hiding this comment

Uh oh!

Warashi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 21, 2025 •

edited

Loading