[SDK] add unit types to use in config types#5959
Conversation
Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5959 +/- ##
==========================================
+ Coverage 28.50% 28.60% +0.10%
==========================================
Files 520 523 +3
Lines 56434 56528 +94
==========================================
+ Hits 16084 16170 +86
- Misses 39083 39089 +6
- Partials 1267 1269 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
It's a bit hard to decide whether we should include this in the SDK or keep these plugins separate and accept the fact that they could be redundant and copied around. I think what is placed in the SDK should be something more like piped care about, not something plugins need. Since we can say it's common, but it may block/confuse other plugins that don't want to use the SDK-defined config structure. WDYT? |
|
Yes, I also think it's hard to decide whether we should put these in the SDK. I noticed we already have a " diff " package; it's not something piped cares about, but a package we thought was commonly used across plugins. |
|
@Warashi I got your point, then for now I only concern for the name of package, but it's not too strong. Let's keep it as is for now 👍 |
t-kikuc
left a comment
There was a problem hiding this comment.
thanks, wait stage needs it too.
What this PR does:
as title
Why we need it:
They are used generally in the platform provider's specific configs in pipedv0, and are useful to define config types.
And I want to use them in the Kubernetes plugin.
In the current implementation of the Kubernetes plugin, we use
pkg/configv1, but it's not a good way.pipecd/pkg/app/pipedv1/plugin/kubernetes/config/baseline.go
Lines 17 to 25 in f39124f
Which issue(s) this PR fixes:
Part of #5530
Does this PR introduce a user-facing change?: No