-
Notifications
You must be signed in to change notification settings - Fork 182
Remove deprecated ReportEventHandled rpc from piped API #5620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: khanhtc1202 <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5620 +/- ##
==========================================
+ Coverage 26.56% 26.57% +0.01%
==========================================
Files 475 475
Lines 50592 50582 -10
==========================================
+ Hits 13438 13444 +6
+ Misses 36091 36075 -16
Partials 1063 1063 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the definitions of request/response messages remain.
Please remove them.
pipecd/pkg/app/server/service/pipedservice/service.proto
Lines 476 to 481 in 771da47
message ReportEventsHandledRequest { | |
repeated string event_ids = 1 [(validate.rules).repeated.min_items = 1]; | |
} | |
message ReportEventsHandledResponse { | |
} |
Signed-off-by: khanhtc1202 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirmed that ReportEventsHandled()
is not called anywhere.
What "old Piped" means is unclear 😓
What this PR does:
as title
Why we need it:
This RPC is no more used by piped(s) from 3 years ago 👴
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: