Skip to content

Implement DetermineVersions with SDK#5657

Merged
ffjlabo merged 3 commits intomasterfrom
re-implement-DetermineVersion-with-SDK
Mar 13, 2025
Merged

Implement DetermineVersions with SDK#5657
ffjlabo merged 3 commits intomasterfrom
re-implement-DetermineVersion-with-SDK

Conversation

@ffjlabo
Copy link
Member

@ffjlabo ffjlabo commented Mar 13, 2025

What this PR does:

I implemented k8s DetermineVersion by using SDK.

Why we need it:

We want to implement plugins with SDK.

Which issue(s) this PR fixes:

Part of #4980 #5006

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

@ffjlabo ffjlabo marked this pull request as ready for review March 13, 2025 02:13
@codecov
Copy link

codecov bot commented Mar 13, 2025

Codecov Report

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

Project coverage is 25.54%. Comparing base (883243c) to head (7d8e4a2).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...app/pipedv1/plugin/kubernetes/deployment/plugin.go 0.00% 17 Missing ⚠️
.../pipedv1/plugin/kubernetes/deployment/determine.go 0.00% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5657      +/-   ##
==========================================
- Coverage   25.55%   25.54%   -0.02%     
==========================================
  Files         477      477              
  Lines       51101    51129      +28     
==========================================
  Hits        13060    13060              
- Misses      37036    37064      +28     
  Partials     1005     1005              

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

Copy link
Member

@Warashi Warashi left a comment

Choose a reason for hiding this comment

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

Thank you!
I commented on logging.

func (p *Plugin) DetermineVersions(context.Context, *sdk.ConfigNone, *sdk.Client, *sdk.DetermineVersionsInput) (*sdk.DetermineVersionsResponse, error) {
return &sdk.DetermineVersionsResponse{}, nil
func (p *Plugin) DetermineVersions(ctx context.Context, _ *sdk.ConfigNone, _ *sdk.Client, input *sdk.DetermineVersionsInput) (*sdk.DetermineVersionsResponse, error) {
lp := input.Client.LogPersister()
Copy link
Member

Choose a reason for hiding this comment

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

We cannot use LogPersister for anything other than ExecuteStage.
We have to use input.Logger instead.

// This method should be called only when the client is working with a specific stage, for example, when this client is passed as the ExecuteStage method's argument.
// Otherwise, it will return nil.
// TODO: we should consider returning an error instead of nil, or return logger which prints to stdout.
func (c *Client) LogPersister() StageLogPersister {

Copy link
Member Author

Choose a reason for hiding this comment

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

@Warashi
Thanks! I forgot that 🙏 and fixed on 743bbd3

@ffjlabo ffjlabo requested a review from Warashi March 13, 2025 02:49
Copy link
Member

@Warashi Warashi left a comment

Choose a reason for hiding this comment

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

Almost LGTM
I commented on nits

"github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/toolregistry"
config "github.com/pipe-cd/pipecd/pkg/configv1"
"github.com/pipe-cd/pipecd/pkg/plugin/sdk"
"go.uber.org/zap"
Copy link
Member

Choose a reason for hiding this comment

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

Please move this import to before the imports like github.com/pipe-cd/pipecd/**

Copy link
Member Author

Choose a reason for hiding this comment

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

@Warashi
Sorry, fixed on 7d8e4a2 and fixed my IDE's import setting 🙏

ffjlabo added 3 commits March 13, 2025 13:18
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>
@ffjlabo ffjlabo force-pushed the re-implement-DetermineVersion-with-SDK branch from 743bbd3 to 7d8e4a2 Compare March 13, 2025 04:35
Copy link
Member

@Warashi Warashi left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@ffjlabo ffjlabo enabled auto-merge (squash) March 13, 2025 04:44
Copy link
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.

👍

@ffjlabo ffjlabo merged commit 358d1b3 into master Mar 13, 2025
16 of 18 checks passed
@ffjlabo ffjlabo deleted the re-implement-DetermineVersion-with-SDK branch March 13, 2025 05:45
@github-actions github-actions bot mentioned this pull request Mar 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants