Add Deploysource as proto model #5112
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>
Warashi
requested review from
nghialv,
khanhtc1202,
kentakozuka,
ffjlabo and
t-kikuc
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5112 +/- ##
==========================================
+ Coverage 22.78% 22.83% +0.05%
==========================================
Files 409 419 +10
Lines 43720 45297 +1577
==========================================
+ Hits 9961 10344 +383
- Misses 32975 34158 +1183
- Partials 784 795 +11 ☔ View full report in Codecov by Sentry. |
pkg/model/deployment_source.proto
Outdated
| // The configuration of the application which is independent for plugins. | ||
| GenericApplicationSpec generic_application_config = 3; | ||
| // The configuration of the application which is specific for plugins. | ||
| PluginApplicationSpec application_config = 4; |
There was a problem hiding this comment.
For these two, I think we need the data structure under the pkg/config package. So, how about passing this as bytes to the plugin, and the plugin will import the pipecd pkg/config package to unmarshal it? In that case, we can reuse the logic of the marshal/unmarshal config we currently have.
There was a problem hiding this comment.
I mean, reuse this whole: https://github.com/pipe-cd/pipecd/blob/master/pkg/config/application.go#L29-L61
There was a problem hiding this comment.
I removed GenericApplicationSpec.
PluginApplicationSpec's fields are api_version, kind, and spec.
The api_version and kind are common for all plugins, so I defined as fields.
The field spec is defined as bytes, as you pointed out.
pkg/model/deployment_source.proto
Outdated
| message GenericApplicationSpec { | ||
| // TODO: Add more fields to support generic application spec. |
There was a problem hiding this comment.
I just realized the plugin does not need the GenericApplicationSpec at all 🤔
There was a problem hiding this comment.
I removed GenericApplicationSpec from the model because the plugin does not need it, as you pointed out.
Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
Warashi
force-pushed
the
deploysource-model
branch
from
dc05ef6
to
1a7d2ee
Compare
|
|
||
| option go_package = "github.com/pipe-cd/pipecd/pkg/model"; | ||
|
|
||
| message DeploymentSource { |
There was a problem hiding this comment.
Note: repoDir is removed because it is only being used in the deploysource package. We will change our way to clone/prepare source later.
pkg/model/deployment_source.proto
Outdated
| // The git commit revision of the source code. | ||
| string revision = 2; | ||
| // The configuration of the application which is specific for plugins. | ||
| PluginApplicationSpec application_config = 3; |
There was a problem hiding this comment.
I don't get the point of having this type of definition instead of just using bytes. Could you help me understand more about this 👀
There was a problem hiding this comment.
I think kind and api_version exist for all configurations, so I defined them separately.
Additionally, these definitions reflect the limitation of the config.
There was a problem hiding this comment.
@Warashi Thanks for the explanation 🙏 I think this place we should not define a new type for lower maintenance costs. It could be better to just use bytes type here and both piped and plugin source code will use the config.ApplicationConfig type and marshall function defined in config package. wdyt? 🤔
There was a problem hiding this comment.
@khanhtc1202
Do you mean config.Config?
I think the plugin source code will not use this because the config struct will be defined in the plugin code, not in the piped code.
There was a problem hiding this comment.
I think this place we should not define a new type for lower maintenance costs.
I got your point 👍🏻
I'll try to define a function or struct/method used by both piped and plugins.
I think this definition will use the generic type parameter.
There was a problem hiding this comment.
@khanhtc1202
I fixed the type as bytes as you pointed.
In this PR, I want to define DeploymentSource only.
I'll implement config.ApplicationConfig or a similar one in another PR when I need it.
Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
What this PR does / why we need it:
Retry of #5050
I noticed that the piped also uses the Deploysource, so I defined it not in the plugin proto but in the model proto.
Which issue(s) this PR fixes:
Part of #4980
Does this PR introduce a user-facing change?: