validate: Refactor linearization handling and visualization in operations.go #19094
Conversation
Refactored validateLinearizableOperationsAndVisualize to remove the anonymous visualization function. Made a Results struct to store LinearizationInfo, Model, and the logger, simplifying the code. Signed-off-by: VidhuSarwal <[email protected]>
validate: refactor linearization handling in operations.go
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: VidhuSarwal The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @VidhuSarwal. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
| Lg *zap.Logger // Include logger in the Results struct | ||
| } | ||
|
|
||
| // Change the Visualize method to match the signature func(path string) error |
There was a problem hiding this comment.
This commend describes the PR, but they will no longer be relevant after we merge it.
| if linearizable != porcupine.Ok { | ||
| t.Error("Failed linearization, skipping further validation") | ||
| return visualize | ||
| return results.Visualize |
There was a problem hiding this comment.
Why not return results struct? It should allow caller to call results.Visualize and futher eliminate usage of arbitrary function interface.
| type Results struct { | ||
| Info porcupine.LinearizationInfo | ||
| Model porcupine.Model | ||
| Lg *zap.Logger // Include logger in the Results struct |
There was a problem hiding this comment.
This comment doesn't do much. Maybe instead we leave a TODO to remove logger from struct and instead of making it an argument for Visualize?
|
High level looks good, just couple of nits. |
|
Tests failed with Think you need to upgrade the porcupine version. |
Signed-off-by: VidhuSarwal <[email protected]>
|
updated porcupine to v1.0.0, Made the change in go.mod (bumped the version number) ran go mod tidy and now getting these errors. test like pull-etcd-e2e-arm64 suggests go.sum entry is missing, but go.sum did make it (evident in changes) so not able to understand what exact issue is. Even manually ran Also one test (robustness) suggests I update the test file to the new return type of |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 25 files with indirect coverage changes @@ Coverage Diff @@
## main #19094 +/- ##
==========================================
- Coverage 68.85% 68.81% -0.04%
==========================================
Files 420 420
Lines 35642 35642
==========================================
- Hits 24541 24527 -14
- Misses 9676 9689 +13
- Partials 1425 1426 +1 Continue to review full report in Codecov by Sentry.
|
|
@VidhuSarwal: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Closes #19089
Changes made:
Refactored operations.go
validateLinearizableOperationsAndVisualizefunctionLinearizationInfo,Model, and the logger.