Implement initialization of piped service client in k8s plugin #5387
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5387 +/- ##
==========================================
- Coverage 25.85% 25.84% -0.02%
==========================================
Files 447 448 +1
Lines 48233 48261 +28
==========================================
Hits 12472 12472
- Misses 34790 34818 +28
Partials 971 971 ☔ View full report in Codecov by Sentry. |
| apiPort: 10000, | ||
| gracePeriod: 30 * time.Second, | ||
| apiPort: 10000, | ||
| pipedPluginServicePort: 9087, |
There was a problem hiding this comment.
[ASK] Where does 9087 come from?
Do other plugins need to avoid using 9087?
There was a problem hiding this comment.
Sorry for not commenting about this.
This comes from the default of pipedv1's plugin service port.
pipecd/pkg/app/pipedv1/cmd/piped/piped.go
Line 103 in 4986dd5
There was a problem hiding this comment.
I got it, thank you.
I think it'd be better to avoid hard coding (e.g., by using const) because all plugins need to specify 9087 directly.
If it's not easy now, let's do it in another PR later.
There was a problem hiding this comment.
I came to one idea.
The piped will always specify the port when invoking the plugin.
Then, making the default -1 and the plugin raises the error if the piped doesn't specify the port.
WDYT?
There was a problem hiding this comment.
It seems good!
You mean a piped passes the piped's port to each plugin, right? If not passed, an error will occur by -1
There was a problem hiding this comment.
Yes 👍🏻
I realized the way to pass this port must be the same across all the plugins.
We have to discuss how to pass it (e.g., as a command option or an environment variable, and these names, too).
Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
| if s.pipedPluginServicePort == -1 { | ||
| input.Logger.Error("piped-plugin-service-port is required") | ||
| return errors.New("piped-plugin-service-port is required") | ||
| } |
There was a problem hiding this comment.
We can use MarkFlagRequired to require the flag 👀
https://pkg.go.dev/github.com/spf13/cobra#MarkFlagRequired
There was a problem hiding this comment.
Then the following line is unnecessary, right?
pipedPluginServicePort: -1, // default as error valueThere was a problem hiding this comment.
oops, that's right. I'll change
There was a problem hiding this comment.
thanks. fixed on this commit
2d1da06
Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
What this PR does:
as title
Why we need it:
We need the piped's plugin service client to call the APIs.
Which issue(s) this PR fixes:
Part of #4980
Does this PR introduce a user-facing change?: No