-
Notifications
You must be signed in to change notification settings - Fork 669
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
Adopt protogetter #5981
Adopt protogetter #5981
Conversation
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Dependency ReviewThe following issues were found:
|
#5977 is going to be fixed by this too. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5981 +/- ##
==========================================
+ Coverage 36.97% 37.03% +0.05%
==========================================
Files 1312 1313 +1
Lines 131525 131621 +96
==========================================
+ Hits 48636 48748 +112
+ Misses 78662 78645 -17
- Partials 4227 4228 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Signed-off-by: Eduardo Apolinario <[email protected]>
Tracking issue
Closes #5980
Why are the changes needed?
We shouldn't panic due to incomplete proto messages.
What changes were proposed in this pull request?
I added https://github.com/ghostiam/protogetter to
golangci-lint
in all components. I removed unused linters and fixed a bunch of warnings.There are 2 places where I left a comment:
flyte/flyteplugins/go/tasks/pluginmachinery/core/exec_metadata.go
Line 30 in 2edf4cb
flyte/flyteplugins/go/tasks/pluginmachinery/core/exec_metadata.go
Line 51 in 2edf4cb
Notice how in both cases we do not return a pointer to a proto struct, which renders the technique used to access potentially nil values in a proto struct invalid, hence the need for
// nolint:protogetter
whenever we access fields in those two objects (e.g.flyte/flyteplugins/go/tasks/plugins/webapi/agent/plugin.go
Line 391 in 2edf4cb
How was this patch tested?
Unit tests and local sandbox.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link