Refactor flyteadmin to pass proto structs as pointers #5717
Conversation
Signed-off-by: Jason Parraga <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5717 +/- ##
==========================================
+ Coverage 36.17% 36.20% +0.03%
==========================================
Files 1303 1303
Lines 109663 109543 -120
==========================================
- Hits 39667 39657 -10
+ Misses 65851 65768 -83
+ Partials 4145 4118 -27
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: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Sovietaced
force-pushed
the
cleanup-warnings
branch
from
6565fc1 to
d1c5a27
Compare
Signed-off-by: Jason Parraga <[email protected]>
| @@ -36,5 +36,6 @@ linters-settings: | |||
| - prefix(github.com/flyteorg) | |||
| skip-generated: true | |||
| issues: | |||
| exclude: | |||
| - copylocks | |||
| exclude-rules: | |||
There was a problem hiding this comment.
Fine grained ignore
There was a problem hiding this comment.
why does this still need to be excluded?
There was a problem hiding this comment.
The code copies the a security context to the flyte workflow custom resource, which stores it as a non-pointer so I think its inevitable unless I write some conversion method to traverse the security context and reconstruct a non-pointer version. I felt it was probably less error prone and more future proof to do a deep copy and ignore the warning.
There was a problem hiding this comment.
thanks for explaining, leaving this as an exception makes sense!
| @@ -78,20 +78,15 @@ func GetValidTaskRequestWithOverrides(project string, domain string, name string | |||
| } | |||
| } | |||
|
|
|||
| func GetValidTaskSpecBytes() []byte { | |||
There was a problem hiding this comment.
This was unused so I just removed it, I think that should be safe since this is a testutils package?
There was a problem hiding this comment.
sounds good and thank you!
davidmirror-ops
added
the
size:XXL
davidmirror-ops
requested review from
wild-endeavor,
eapolinario and
EngHabu
| @@ -36,5 +36,6 @@ linters-settings: | |||
| - prefix(github.com/flyteorg) | |||
| skip-generated: true | |||
| issues: | |||
| exclude: | |||
| - copylocks | |||
| exclude-rules: | |||
There was a problem hiding this comment.
why does this still need to be excluded?
Signed-off-by: pmahindrakar-oss <[email protected]>
Signed-off-by: Bugra Gedik <[email protected]>
Why are the changes needed?
Go structs generated by
protoc-gen-gocontain a DoNotCopy mutex so thatgo vetwill complain and discourage people from copying these go structs.The
flyteadmincode doesn't respect these intentions and has many of these warnings. 1,135 to be exact. The orange warnings are a bit noisy in the IDE and I noticed thegolangciconfig ignores them.As someone who comes from a Java background I'm guessing that the intent of passing around copies of these go structs is to prevent nil pointer errors and employ defensive programming. I think in most circumstances this makes sense, but in this case gRPC and
protoc-gen-goare very intentional about using pointers. The structs are never nil in the gRPC API handlers, they are just passed with pointers so that the structs are used safely.Having said that, this pull request attempts to refactor
flyteadminto use pointers for protobuf structs throughout the codebase to eliminate these warnings. It updates thegolangciconfig to ignore the singular unavoidable and safe copy that is made.see: https://stackoverflow.com/questions/64183794/why-do-the-go-generated-protobuf-files-contain-mutex-locks
How was this patch tested?
Unit tests and integration test have been updated.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link