[SDK] Add type parameter for DeploymentSource and unmarshalling the spec #5740

Warashi · 2025-04-16T07:22:15Z

What this PR does:

This PR adds a type parameter to the DeploymentSource struct and Unmarshalling app.pipecd.yaml when invoking the plugins' methods.

Why we need it:

I realized we can do this, and I don't want each plugin developer to write config unmarshalling code.

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: Shinnosuke Sawada-Dazai <[email protected]>

…nfig. Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

…ginServiceServer. Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

codecov · 2025-04-16T08:48:22Z

Codecov Report

Attention: Patch coverage is 34.73684% with 124 lines in your changes missing coverage. Please review.

Project coverage is 26.98%. Comparing base (4ca4a01) to head (4885a40).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/plugin/sdk/deployment.go	43.54%	30 Missing and 5 partials ⚠️
pkg/plugin/sdk/deployment_source.go	46.66%	18 Missing and 6 partials ⚠️
pkg/plugin/sdk/plugin.go	0.00%	15 Missing ⚠️
pkg/plugin/sdk/sdktest/config.go	0.00%	13 Missing ⚠️
...app/pipedv1/plugin/kubernetes/deployment/plugin.go	12.50%	7 Missing ⚠️
...lugin/kubernetes_multicluster/deployment/plugin.go	12.50%	7 Missing ⚠️
.../app/pipedv1/plugin/kubernetes/livestate/plugin.go	0.00%	4 Missing ⚠️
...plugin/kubernetes_multicluster/livestate/plugin.go	0.00%	4 Missing ⚠️
...p/pipedv1/plugin/kubernetes/deployment/baseline.go	0.00%	2 Missing ⚠️
...app/pipedv1/plugin/kubernetes/deployment/canary.go	0.00%	2 Missing ⚠️
... and 10 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5740      +/-   ##
==========================================
- Coverage   26.98%   26.98%   -0.01%     
==========================================
  Files         503      504       +1     
  Lines       53047    53120      +73     
==========================================
+ Hits        14317    14335      +18     
- Misses      37665    37709      +44     
- Partials     1065     1076      +11

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

khanhtc1202

Got the point, looks neat 👍

ffjlabo

Thank you for the nice improvement! This is what I also expected.
I left some comments.

ffjlabo · 2025-04-16T10:03:29Z

pkg/plugin/sdk/deployment_source.go

+type ApplicationConfig[Spec any] struct {
+	commonSpec *config.GenericApplicationSpec
+	Spec       *Spec
+}


[nit] plz add comment for the struct and fields 🙏

Added comments
c92050d

ffjlabo · 2025-04-16T10:14:55Z

pkg/app/pipedv1/plugin/kubernetes/deployment/rollback_test.go

+	cfgBytes, err := os.ReadFile(filepath.Join(examplesDir(), "kubernetes", "simple", "app.pipecd.yaml"))
+	require.NoError(t, err)
+	cfg, err := sdk.DecodeYAML[kubeConfigPkg.KubernetesApplicationSpec](cfgBytes)


[imo]
It would be nice to define a helper function to create ApplicationConfigSpec from a file or string.
I feel users don't need to be aware of sdk.DecodeYAML in the actual logic.
WDYT?

I agree with you, so I added sdk.LoadApplicationConfig and removed sdk.DecodeYAML
5abbdd5

@Warashi
Thank you for adding the helper method!
I read the comment and think that we can make a test package sdk/sdktest . How about you?

/ LoadApplicationConfig loads the application config from the given filename. // This is intended to use in the tests.

@ffjlabo

It sounds good.
How about making the sdktest.LoadApplicationConfig 's first argument t *testing.T and automatically calling t.Fatal when it meets an unmarshalling error and making its return value only *ApplicationConfig[Spec]?

@Warashi Sounds great!!! Let's go with that!

Implemented and switched the usage.
30bd753
4885a40

Sounds nice! That's a nice improvement!

ffjlabo · 2025-04-16T10:18:31Z

pkg/app/pipedv1/plugin/kubernetes/deployment/plugin.go

-	if err != nil {
-		logger.Error("Failed while decoding application config", zap.Error(err))
-		return nil, err
+	cfg := input.Request.TargetDeploymentSource.ApplicationConfig.Spec


[imo] It would be nice to add a method into DeploySource to get ApplicationConfig from DeploySource, including a nil check because ApplicationConfig is a pointer.
WDYT?

I agree with you and I added AppConfig method
53ef850

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

ffjlabo

LGTM!

Warashi · 2025-04-17T01:31:02Z

@khanhtc1202 Sorry, please re-review

khanhtc1202

LNeatTM 👌

Warashi added 7 commits April 16, 2025 15:37

Refactor import statement in config.go for improved readability

e5dda81

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Refactor the deployment source into a separate file.

f9e47e0

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add ApplicationConfig type to make it easier to handle application co…

629d27e

…nfig. Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Introduce type parameters to DeploymentSource.

e1c797b

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Refactor to use type parameters for application config spec.

77fb2c1

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add type parameters to the DeploymentPluginServiceServer and StagePlu…

9b2aef2

…ginServiceServer. Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add type parameters to DeploymentPlugin

3dd620a

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

github-actions bot added the area/go label Apr 16, 2025

Warashi added 17 commits April 16, 2025 16:30

Fix the type parameters of the plugin.

7624171

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the type parameters of the LivestatePlugin.

f2e80ad

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the type of the livestate plugin.

419eab1

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the type errors in the test files

afc87ac

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the type of the application config spec in the example plugin.

e9fbf82

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the type errors in the Kubernetes deployment plugin.

900f69f

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix type parameters for the application config

1e28d94

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add DecodeYAML method to the deployment source

03cecc8

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the test cases

b3ff4ce

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the test cases

2405095

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the type of the application config in the livestate plugin.

91a7527

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the type of the input for the wait plugin

b0f18f8

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the type parameters of the deployment plugin

1a0fdbb

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the test in kubernetes_multicluster plugin

fd6e873

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the type of the application config in the rollback stage.

797e588

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the type parameters of the plugin SDK.

dccdae5

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the lint notification

be77e83

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

github-actions bot added the area/pipedv1 label Apr 16, 2025

Warashi added 4 commits April 16, 2025 17:16

Remove empty lines

0cec7a9

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the test

384a0f9

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the test for DetermineVersionsRequest

e710fd4

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the test for the kubernetes deployment plugin

d152c23

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi added 2 commits April 16, 2025 17:44

Fix test cases

2f52c38

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the livestate test

3ed79a2

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi marked this pull request as ready for review April 16, 2025 08:50

Warashi requested review from khanhtc1202, ffjlabo and t-kikuc as code owners April 16, 2025 08:50

khanhtc1202 previously approved these changes Apr 16, 2025

View reviewed changes

ffjlabo reviewed Apr 16, 2025

View reviewed changes

Warashi added 3 commits April 17, 2025 08:55

Add documentation for the ApplicationConfig type.

c92050d

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add LoadApplicationConfig and use it in the tests

5abbdd5

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add AppConfig method to DeploymentSource and use it

53ef850

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi dismissed khanhtc1202’s stale review via 53ef850 April 17, 2025 00:14

Warashi requested review from ffjlabo and khanhtc1202 April 17, 2025 00:19

Warashi added 2 commits April 17, 2025 10:16

Add a helper function to load application config for tests

30bd753

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Switch to use sdktest.LoadApplicationConfig for tests

4885a40

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

ffjlabo approved these changes Apr 17, 2025

View reviewed changes

Warashi enabled auto-merge (squash) April 17, 2025 01:30

khanhtc1202 approved these changes Apr 17, 2025

View reviewed changes

Warashi merged commit d7d637b into master Apr 17, 2025
21 of 22 checks passed

Warashi deleted the plugin-sdk-app-spec-type-params branch April 17, 2025 03:25

This was referenced Apr 17, 2025

[SDK] Add feature to check the pipeline has specific stage #5737

Merged

[K8s Plugin] Refactor to use LoadConfigSpec to load application config #5733

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SDK] Add type parameter for DeploymentSource and unmarshalling the spec #5740

[SDK] Add type parameter for DeploymentSource and unmarshalling the spec #5740

Warashi commented Apr 16, 2025 •

edited

Loading

codecov bot commented Apr 16, 2025 •

edited

Loading

khanhtc1202 left a comment

ffjlabo left a comment

ffjlabo Apr 16, 2025

Warashi Apr 17, 2025

ffjlabo Apr 16, 2025

Warashi Apr 17, 2025

ffjlabo Apr 17, 2025

Warashi Apr 17, 2025

ffjlabo Apr 17, 2025

Warashi Apr 17, 2025

ffjlabo Apr 17, 2025

ffjlabo Apr 16, 2025

Warashi Apr 17, 2025

ffjlabo left a comment

Warashi commented Apr 17, 2025

khanhtc1202 left a comment

[SDK] Add type parameter for DeploymentSource and unmarshalling the spec #5740

[SDK] Add type parameter for DeploymentSource and unmarshalling the spec #5740

Conversation

Warashi commented Apr 16, 2025 • edited Loading

codecov bot commented Apr 16, 2025 • edited Loading

Codecov Report

khanhtc1202 left a comment

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

Warashi commented Apr 17, 2025

khanhtc1202 left a comment

Choose a reason for hiding this comment

Warashi commented Apr 16, 2025 •

edited

Loading

codecov bot commented Apr 16, 2025 •

edited

Loading