Split the annotations package to pieces for Ingress, NEG and L4 #3017
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
|
/assign @TortillaZHawaii |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
TortillaZHawaii
left a comment
TortillaZHawaii
left a comment
There was a problem hiding this comment.
Just a small request to change pkg/annotations to pkg/l7annotations or whatever name that would suggest that it is only for L7 application load balancers.
| // Service represents Service annotations. | ||
| type Service struct { | ||
| v map[string]string | ||
| } | ||
|
|
||
| // FromService extracts the annotations from an Service definition. | ||
| func FromService(obj *v1.Service) *Service { | ||
| return &Service{obj.Annotations} | ||
| } |
There was a problem hiding this comment.
This looks like an awkward API now. To get actual information related to NEGs we need to first call FromService, get *Service object and then call .NEGAnnotation or .NEGStatus on it. Since this PR only moves it around I would leave it, but I think it will lead to confusion for later users.
Also it's hard for me to decipher why some names use full caps NEG while others Neg. It should be NEG when it's in the middle of the word, or is exported; or neg if it's the first word and it's internal.
There was a problem hiding this comment.
I don't know the answers to these questions or more, why someone decided to do it this way.
We can follow up with changing this code and after this CL we can easily change L4 and NEG but leave Ingress alone
| @@ -1 +1 @@ | |||
| /* | |||
There was a problem hiding this comment.
Can the package be renamed to l7annotations? It could be non obvious for others that the L4 annotations shouldn't go to annotations anymore.
|
|
||
| // GetL4LoggingConfigMapAnnotation returns name of the config map which contains logging config. | ||
| // Returns false if annotation with the name is not specified. | ||
| func (svc *Service) GetL4LoggingConfigMapAnnotation() (string, bool) { |
There was a problem hiding this comment.
it's confusing to me why some func in this file use *Service, and others *v1.Service. Since this PR is just moving stuff let's leave it, but for the end user of this functions it looks awkward.
There was a problem hiding this comment.
I don't know why it was made this way but Service is a type inside this file as just annotations container while v1.Service is the full k8s resource... :/
k8s-ci-robot
added
the
needs-rebase
mmamczur
force-pushed
the
split_l4_neg_ingress
branch
from
b33838b to
a8b1dcc
Compare
k8s-ci-robot
removed
the
needs-rebase
|
/assign @kl52752 |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kl52752, mmamczur 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 |
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
4 participants
This just moves some types/functions to different packages.