Skip to content

Add default response of DetermineStrategy() of DeploymentPlugin#5929

Merged
t-kikuc merged 2 commits into
masterfrom
sdk/default-strategy
Jun 9, 2025
Merged

Add default response of DetermineStrategy() of DeploymentPlugin#5929
t-kikuc merged 2 commits into
masterfrom
sdk/default-strategy

Conversation

@t-kikuc
Copy link
Copy Markdown
Member

@t-kikuc t-kikuc commented Jun 9, 2025

What this PR does:

  • DetermineStrategy() returns PipelineSync by default
  • Describe the spec of DetermineStrategy()

cf. pipedv0 uses pipelineSync by default.

// Force to use pipeline when the alwaysUsePipeline field was configured.
if cfg.Planner.AlwaysUsePipeline {
out.SyncStrategy = model.SyncStrategy_PIPELINE
out.Stages = buildProgressivePipeline(cfg.Pipeline, cfg.Input.AutoRollback, time.Now())
out.Summary = "Sync with the specified pipeline (alwaysUsePipeline was set)"
return
}
out.SyncStrategy = model.SyncStrategy_PIPELINE
out.Stages = buildProgressivePipeline(cfg.Pipeline, cfg.Input.AutoRollback, now)
out.Summary = "Sync with the specified progressive pipeline"
return
}

Why we need it:

  • Prevent plugin developers from getting confused. They can just return (nil, nil) for DetermineStrategy().
    • Plugins other than the k8s plugin probably do not have the specific logic.

Which issue(s) this PR fixes:

Part of #5530

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):

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
@t-kikuc t-kikuc requested a review from a team as a code owner June 9, 2025 03:38
@github-actions github-actions Bot added area/go area/plugin-sdk piped-plugin-sdk labels Jun 9, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 9, 2025

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 28.45%. Comparing base (0afedaa) to head (c5f6d75).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
pkg/plugin/sdk/deployment.go 0.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5929      +/-   ##
==========================================
- Coverage   28.46%   28.45%   -0.01%     
==========================================
  Files         520      520              
  Lines       56408    56416       +8     
==========================================
  Hits        16055    16055              
- Misses      39087    39095       +8     
  Partials     1266     1266              
Flag Coverage Δ
. 23.73% <ø> (ø)
.-pkg-app-pipedv1-plugin-example 0.00% <ø> (ø)
.-pkg-app-pipedv1-plugin-kubernetes 66.42% <ø> (ø)
.-pkg-app-pipedv1-plugin-kubernetes_multicluster 67.51% <ø> (ø)
.-pkg-app-pipedv1-plugin-wait 35.51% <ø> (ø)
.-pkg-plugin-sdk 49.20% <0.00%> (-0.22%) ⬇️
.-tool-actions-gh-release 19.23% <ø> (ø)
.-tool-actions-plan-preview 25.30% <ø> (ø)
.-tool-codegen-protoc-gen-auth 0.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

@ffjlabo
Copy link
Copy Markdown
Member

ffjlabo commented Jun 9, 2025

@t-kikuc
I think we need to modify the caller logic to handle nil.
This might cause panic because of nil pointer dereference.
https://github.com/pipe-cd/pipecd/blob/a19732a292b9407c8ac03963d71e39a0347329d8/pkg/app/pipedv1/controller/planner.go#L375C1-L388C3

@t-kikuc
Copy link
Copy Markdown
Member Author

t-kikuc commented Jun 9, 2025

@ffjlabo
I think that should be done in another PR as a controller bug.

In this PR, even if DetermineStrategy() of a plugin returns nil, the SDK will not return nil to piped.
(Now, if a plugin returns nil, the SDK will panic.)

@ffjlabo
Copy link
Copy Markdown
Member

ffjlabo commented Jun 9, 2025

@t-kikuc
Sorry, I misunderstood. 🙏 I got your point.

ffjlabo
ffjlabo previously approved these changes Jun 9, 2025
Copy link
Copy Markdown
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment thread pkg/plugin/sdk/deployment.go Outdated
return nil, status.Errorf(codes.Internal, "failed to determine strategy: %v", err)
}
if response == nil {
// when a plugin does not have specific logic.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// when a plugin does not have specific logic.
// If the plugin does not have specific logic to determine strategy
// returns PipelineSync as default.

nits, the comment should be clear, not half-phase, to reduce confusion.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khanhtc1202
Thanks, I updated!
c5f6d75

@t-kikuc t-kikuc changed the title Add default response of DetermineStrategy() Add default response of DetermineStrategy() of DeploymentPlugin Jun 9, 2025
Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>
@t-kikuc t-kikuc enabled auto-merge (squash) June 9, 2025 07:10
Copy link
Copy Markdown
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

@t-kikuc
Copy link
Copy Markdown
Member Author

t-kikuc commented Jun 9, 2025

@ffjlablo
Sorry, please approve again 🙏
And I opened a PR for the issue you mentioned: #5930

@t-kikuc t-kikuc merged commit e1fcf93 into master Jun 9, 2025
39 of 40 checks passed
@t-kikuc t-kikuc deleted the sdk/default-strategy branch June 9, 2025 08:14
@github-actions github-actions Bot mentioned this pull request Jul 14, 2025
@github-actions github-actions Bot mentioned this pull request Aug 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants