Updated grpc2 to grpc3 #9194
Updated grpc2 to grpc3 #9194
Conversation
Signed-off-by: Andrii Chubatiuk <andrew.chubatiuk@gmail.com>
Signed-off-by: Andrii Chubatiuk <andrew.chubatiuk@gmail.com>
Signed-off-by: Andrii Chubatiuk <andrew.chubatiuk@gmail.com>
Signed-off-by: Andrii Chubatiuk <andrew.chubatiuk@gmail.com>
server/application/application.go
Outdated
| @@ -717,16 +705,16 @@ func (s *Server) Delete(ctx context.Context, q *application.ApplicationDeleteReq | |||
| return nil, err | |||
| } | |||
|
|
|||
| if q.Cascade != nil && !*q.Cascade && q.GetPropagationPolicy() != "" { | |||
| if !q.Cascade && q.GetPropagationPolicy() != "" { | |||
There was a problem hiding this comment.
I'm concerned about the 2->3 proto upgrade because we've lost the ability to detect when boolean fields were omitted as opposed to defaulted false. Argo CD has logic that treats omitted fields differently than false, such as the original code here. I believe this is changing the behavior of cascaded deletion here.
There was a problem hiding this comment.
Please see more information about this design decision in proto3:
protocolbuffers/protobuf#359
There was a problem hiding this comment.
OK, so @alexmt just informed me that starting with proto 3.15, fields can once again be marked as optional. I think what needs to happen in this PR is to specify optional to fields like Cascade, so that we get back the original behavior. I believe this will generate Has() methods to indicate if the field was explicitly set or not.
server/application/application.go
Outdated
| validate = *q.Validate | ||
| } | ||
| err := s.validateAndNormalizeApp(ctx, a, validate) | ||
| err := s.validateAndNormalizeApp(ctx, a, q.Validate) |
There was a problem hiding this comment.
Validate is another field where omission matters. Omitted actually equates to true.
server/application/application.go
Outdated
| validate = *q.Validate | ||
| } | ||
| return s.validateAndUpdateApp(ctx, q.Application, false, validate) | ||
| return s.validateAndUpdateApp(ctx, q.Application, false, q.Validate) |
There was a problem hiding this comment.
Another: omitted q.Validate is actually true
server/application/application.go
Outdated
| validate = *q.Validate | ||
| } | ||
| a, err = s.validateAndUpdateApp(ctx, a, false, validate) | ||
| a, err = s.validateAndUpdateApp(ctx, a, false, q.Validate) |
There was a problem hiding this comment.
Another: omitted q.Validate is actually true
Signed-off-by: Andrii Chubatiuk <andrew.chubatiuk@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #9194 +/- ##
==========================================
- Coverage 45.73% 45.71% -0.02%
==========================================
Files 220 220
Lines 26006 26004 -2
==========================================
- Hits 11893 11887 -6
- Misses 12456 12459 +3
- Partials 1657 1658 +1
Continue to review full report at Codecov.
|
Signed-off-by: Andrii Chubatiuk <andrew.chubatiuk@gmail.com>
Signed-off-by: Andrii Chubatiuk <andrew.chubatiuk@gmail.com>
|
I've also tried @jessesuen 's suggestion and also found that gogoprotobuf does not support optional fields :( We plan to drop gogoprotobuf but blocked because k8s protobuf definitions are not compatible with most recent golang proto generator. Kubernetes 1.24 should resolve it |
Fixes #9193 #9196
Gitea tests are failing cause problems in gitea.com
https://gitea.com/api/v1/repos/gitea/test-openldap returns
maindefault branch buthttps://gitea.com/api/v1/repos/gitea/test-openldap/branches has
masteronly