-
Notifications
You must be signed in to change notification settings - Fork 10
Add Reports type to outcome. Currently unused. #892
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
base: main
Are you sure you want to change the base?
Conversation
execute/exectypes/outcome.go
Outdated
// NewSortedOutcome ensures canonical ordering of the outcome. | ||
// TODO: handle canonicalization in the encoder. | ||
func NewSortedOutcome( |
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.
This function doesn't make sense. The pending commits and report types are aligned.
8fd1337
to
2f89eed
Compare
e87b90f
to
3a96e1d
Compare
5c42394
to
8ffeb47
Compare
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.
Pull Request Overview
This PR introduces a new Reports field for outcomes, updating the proto definitions, codec translation functions, and test cases while preserving backward compatibility with the legacy Report field.
- Renamed conversion functions (e.g. chainReportsToProto → execPluginReportsToProto) to support multiple reports.
- Updated proto messages and Go pb code to deprecate and add fields for execute_plugin_report(s).
- Adjusted outcome construction and decoding, and added tests for backward compatibility.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
pkg/ocrtypecodec/v1/translate.go | Renamed translation functions and updated inner loop processing. |
pkg/ocrtypecodec/v1/ocrtypes.proto | Added a repeated field for execute_plugin_reports; deprecated the singular field. |
pkg/ocrtypecodec/v1/ocrtypecodecpb/ocrtypes.pb.go | Updated proto Go bindings to include new methods and fields. |
pkg/ocrtypecodec/v1/exec.go | Revised outcome encoding/decoding for legacy compatibility handling. |
pkg/ocrtypecodec/v1/compatability_test.go | Added tests verifying backward compatibility of outcome encoding. |
execute/tracked.go | Added a TODO to support the new Reports field. |
execute/plugin.go | Updated import for metrics (now using executemetrics). |
execute/exectypes/outcome.go | Introduced a new Reports field in Outcome and adjusted outcome constructors. |
Comments suppressed due to low confidence (1)
pkg/ocrtypecodec/v1/compatability_test.go:63
- The backward compatibility test uses the same base64 encoded outcome for both test cases. Consider adding additional tests that validate the encoding when multiple reports are provided to ensure that the new Reports field is exercised.
encodedOutcome: "Cgd5ZWUgaGF3EjMIYxoCCAkgyQEqIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAMgYI+gEQrAIaBAoCCHs=",
pkg/ocrtypecodec/v1/exec.go
Outdated
pbObs.ExecutePluginReport = pbObs.ExecutePluginReports[0] | ||
pbObs.ExecutePluginReports = nil |
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.
Ensure the legacy compatibility branch properly handles cases where both legacy and new report formats might be present, and add tests to confirm that the conversion logic behaves as expected when the input outcome provides exactly one report versus multiple reports.
pbObs.ExecutePluginReport = pbObs.ExecutePluginReports[0] | |
pbObs.ExecutePluginReports = nil | |
// Ensure legacy compatibility without clearing the new format field. | |
if pbObs.ExecutePluginReport == nil { | |
pbObs.ExecutePluginReport = pbObs.ExecutePluginReports[0] | |
} |
Copilot uses AI. Check for mistakes.
// NewSortedOutcome ensures canonical ordering of the outcome. | ||
// TODO: handle canonicalization in the encoder. | ||
// TODO: this sorting doesn't make sense for all states. |
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.
[nitpick] Revisit the sorting logic in NewSortedOutcome to ensure that the ordering of outcomes remains predictable when multiple reports are provided. Clarify in documentation how the sorting should behave across different plugin states.
Copilot uses AI. Check for mistakes.
No description provided.