Terratest checking Local DNSEndpoint #918
Conversation
related to #50 implemented code that adds labels to the local dnsEndpoint. - covered by unit-tests - the labels are as follows: ```yaml kind: GSLB spec: ingress: rules: - host: roundrobin.cloud.example.com http: # This section mirrors the same structure as that of an Ingress resource and will be used verbatim when creating the corresponding Ingress resource that will match the GSLB host paths: - path: / backend: service: name: existing-app # Gslb should reflect NotFound status port: name: http spec: strategy: roundRobin weight: eu: 35% us: 50% za: 15% ``` has local DNS endpoint like this: ```yaml apiVersion: externaldns.k8s.io/v1alpha1 kind: DNSEndpoint metadata: name: app_host annotations: k8gb.absa.oss/dnstype: local spec: endpoints: - dnsName: app.bar.com labels: weight-eu-0-35: 10.10.0.1 weight-eu-1-35: 10.10.0.2 weight-us-0-50: 10.0.0.1 weight-us-1-50: 10.0.0.2 weight-za-0-15: 10.22.0.1 weight-za-1-15: 10.22.0.2 weight-za-2-15: 10.22.1.1 recordTTL: 180 recordType: A Targets: - 10.10.0.1 - 10.10.0.2 - 10.0.0.1 - 10.0.0.2 - 10.22.0.1 - 10.22.0.2 - 10.22.1.1 ``` Signed-off-by: kuritka <[email protected]>
Please review after #914 merged I had to prepare one extra terratest to verify that #914 is creating labels correctly. The terratests that control the weight will come later. Together with the test I extended the utils with DNSEndpoint class, which returns information about DNS endpoint. Signed-off-by: kuritka <[email protected]>
kuritka
requested review from
donovanmuller,
k0da,
somaritane,
ytsarev and
jkremser
as code owners
kuritka
force-pushed
the
wrr-3-annotation-merge-terratest
branch
from
f9204d3 to
994dc95
Compare
controllers/dnsupdate.go
Outdated
| @@ -166,3 +161,19 @@ func (r *GslbReconciler) updateRuntimeStatus(gslb *k8gbv1beta1.Gslb, isPrimary b | |||
| m.UpdateFailoverStatus(gslb, isPrimary, isHealthy, finalTargets) | |||
| } | |||
| } | |||
|
|
|||
| // getLabels map of where key identifies region and weight, value identifies IP. | |||
| func (r *GslbReconciler) getLabels(gslb *k8gbv1beta1.Gslb, targets assistant.Targets) (labels map[string]string) { | |||
There was a problem hiding this comment.
Shouldn't that be named genLabels instead? getLables quite confusing.
There was a problem hiding this comment.
Hi @k0da, thx for good comment. I will try to explain: Yes it generates labels and therefore it should be called genLabels. getLabels is getter function that always returns a label map.
The main reason why I would choose get over gen. Overall practice is getters don't change state. A getter returns or generates a value, but it doesn't change the state (for example, if the getLabels function somewhere inside would somehow change the endpoint, or gslb etc....). A getter can't change anything, just read anything and return the value or modified or generated value.
This reduces the cognitive load a lot, because if something is a getter, the developer doesn't have to worry about the details of how the get works. He doesn't have to study the body of the function in order to have it change some instances.
Imagine, In the case of genLabels, as someone who did not write the code, I would be worried that the function inside might change the state (e.g: modify something). I'd have to go in and study the details of the code, to ensure it reads only. Even though it's not related to the thing I'm working on.
There was a problem hiding this comment.
Makes sense, my assumption was getter gets it from external source
✅ Deploy Preview for k8gb-preview ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Please review after #914 merged
I had to prepare one extra terratest to verify that #914 is creating labels correctly. The terratests that control the weight will come later. Together with the test I extended the utils with DNSEndpoint class, which returns information about DNS endpoint.
Signed-off-by: kuritka [email protected]