Support for optional Ingress strategy annotations #445
Conversation
ytsarev
requested review from
donovanmuller,
k0da,
kuritka and
somaritane
as code owners
| @@ -61,6 +61,8 @@ func TestK8gbIngressAnnotationFailover(t *testing.T) { | |||
| assertGslbStatus(t, options, "test-gslb-annotation-failover", "notfound.cloud.example.com:NotFound roundrobin.cloud.example.com:NotFound unhealthy.cloud.example.com:NotFound") | |||
| assertGslbSpec(t, options, "test-gslb-annotation-failover", ".spec.strategy.type", "failover") | |||
| assertGslbSpec(t, options, "test-gslb-annotation-failover", ".spec.strategy.primaryGeoTag", "eu") | |||
| assertGslbSpec(t, options, "test-gslb-annotation-failover", ".spec.strategy.dnsTtlSeconds", "60") | |||
There was a problem hiding this comment.
aren't values supposed to be ints?
There was a problem hiding this comment.
@k0da yeah, good point, but it checks with kubectl based jsonpath behind the scenes, so everything is string in the e2e test context. See https://github.com/AbsaOSS/k8gb/blob/master/terratest/test/helpers.go#L159-L164 for details
controllers/gslb_controller.go
Outdated
| strToInt := func(string string) int { | ||
| intValue, err := strconv.Atoi(string) | ||
| if err != nil { | ||
| log.Err(err) |
There was a problem hiding this comment.
logger will not print message, use
log.Err(err).Msg("")
// or
log.Err(err).Msg(use some text....)There was a problem hiding this comment.
@kuritka amended, thanks
controllers/gslb_controller.go
Outdated
| @@ -240,6 +243,23 @@ func (r *GslbReconciler) SetupWithManager(mgr ctrl.Manager) error { | |||
| }, | |||
| } | |||
|
|
|||
| strToInt := func(string string) int { | |||
There was a problem hiding this comment.
func(string string) looks a bit confusing, consider renaming the parameter, so it doesn't clash with the type name)
There was a problem hiding this comment.
For better readability, consider to put function declaration to the beginning of the caller function (between variable, constant declarations).
There was a problem hiding this comment.
@kuritka could you point me to the location you mean? I'm not 100% sure I'm getting you right
There was a problem hiding this comment.
@somaritane renamed to str, amended)
There was a problem hiding this comment.
@ytsarev basically is up to you. You can keep it as it is, because the function is preceded only by some initializations. Also strToInt is used only in context of createGslbFromIngress handler and nowhere else.
Another place to put this func is under SetupWithManager, so it is at the same level as other funcs.
Both seems totally equal.
There was a problem hiding this comment.
Also strToInt is used only in context of createGslbFromIngress handler and nowhere else.
That's exactly why I decided to declare it inline and next to the actual invocation.
I think I will keep it as it is for now and we move it out to some other level when/if there will be another case of strToInt usage in the codebase.
Thanks a lot for a careful review.
* Cover additional Gslb spec options with annotated Ingress * Resolves #316 Signed-off-by: Yury Tsarev <[email protected]>
ytsarev
force-pushed
the
annotation-spec
branch
from
a012efa to
afa2473
Compare
k8gb.io/dns-ttl-secondsandk8gb.io/splitbrain-threshold-secondsstrategy annotations #316Signed-off-by: Yury Tsarev [email protected]