Add deploy_targets field to GetLivestateRequest message#5618
Add deploy_targets field to GetLivestateRequest message#5618khanhtc1202 merged 1 commit intomasterfrom
Conversation
Signed-off-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5618 +/- ##
=======================================
Coverage 26.57% 26.57%
=======================================
Files 475 475
Lines 50592 50592
=======================================
Hits 13444 13444
Misses 36085 36085
Partials 1063 1063 ☔ View full report in Codecov by Sentry. |
t-kikuc
left a comment
t-kikuc
left a comment
There was a problem hiding this comment.
Seems good! I wanna ask you one tiny point!
| message GetLivestateRequest { | ||
| string application_id = 1; | ||
| grpc.plugin.common.v1alpha1.DeploymentSource deploy_source = 2; | ||
| repeated string deploy_targets = 3; |
There was a problem hiding this comment.
[ask] Are these fields required or not?
There was a problem hiding this comment.
I think the application ID seems required, but the others are plugin-dependent.
There was a problem hiding this comment.
IMO, all should be required since this is livestate, so common plugins (like wait, wait approval) would not use this; only platform-based plugins use these fields, so should mark as required, wdyt?
There was a problem hiding this comment.
I agree with you about the plugins I can imagine now. But I think we should not restrict the possibility of third-party plugins that don't use these fields.
3 participants
What this PR does:
as title
Why we need it:
We need the deploy target information to retrieve live resource states.
Which issue(s) this PR fixes:
Part of #5363
Does this PR introduce a user-facing change?: No.