fix and refactor on NEG annotation handling #452
Conversation
k8s-ci-robot
added
size/L
pkg/annotations/service.go
Outdated
|
|
||
| // NegAnnotation returns the value of the NEG annotation key | ||
| func (svc *Service) NegAnnotation() (NegAnnotation, error) { | ||
| // GetNEGAnnotation returns if NEG annotation is found. |
There was a problem hiding this comment.
Nit - returns true, the NEG annotation if the NEG annotation is found
pkg/annotations/service.go
Outdated
| func (svc *Service) NegAnnotation() (NegAnnotation, error) { | ||
| // GetNEGAnnotation returns if NEG annotation is found. | ||
| // If found, it also returns NEG annotation struct. | ||
| func (svc *Service) GetNEGAnnotation() (bool, *NegAnnotation, error) { |
There was a problem hiding this comment.
I think go style is to just have it as NEGAnnotation() rather than GetNEGAnnotation()
|
LGTM |
pkg/annotations/service.go
Outdated
| var ( | ||
| ErrBackendConfigNoneFound = errors.New("no BackendConfig's found in annotation") | ||
| ErrBackendConfigInvalidJSON = errors.New("BackendConfig annotation is invalid json") | ||
| ErrBackendConfigAnnotationMissing = errors.New("BackendConfig annotation is missing") | ||
| NEGAnnotationErrorFmt = "NEG annotation is invalid: %v" |
There was a problem hiding this comment.
I don't think its worth surfacing the actual formatting error to users. They can easily figure out what's wrong by throwing it into a json parser. That way, in your tests, you don't have to check for a specific error string.
| @@ -79,7 +79,11 @@ func (t *Translator) getServicePort(id utils.ServicePortID) (*utils.ServicePort, | |||
| return nil, errors.ErrSvcPortNotFound{ServicePortID: id} | |||
| } | |||
|
|
|||
| negEnabled := annotations.FromService(svc).NEGEnabled() | |||
| var negEnabled bool | |||
| ok, negAnnotation, err := annotations.FromService(svc).GetNEGAnnotation() | |||
There was a problem hiding this comment.
What if there is an error? Where is that handled?
|
Fixed. PTAL |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: freehan, rramkumar1 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Cherrypick #452 into release-1.3
Refactor NEG annotation handling
Fix a problem where NEG annotation mal format errors are eaten.
cc: @agau4779