Add env variable to set default dns TTL #1668
Conversation
Boomatang
left a comment
Boomatang
left a comment
There was a problem hiding this comment.
We need to address the error handling if the env vars can not be parsed into ints
| func DNSPolicyDefaultTTL() int { | ||
| var ttl, _ = env.GetInt("DNS_DEFAULT_TTL", builder.DefaultTTL) | ||
| return ttl | ||
| } | ||
|
|
||
| func DNSPolicyDefaultCnameTTL() int { | ||
| var ttl, _ = env.GetInt("DNS_DEFAULT_LB_TTL", builder.DefaultLoadBalancedTTL) | ||
| return ttl | ||
| } |
There was a problem hiding this comment.
There is a few things that I am not happy about with this.
I don't like being able to look up environment at stage in the controllers life cycle, but there has being a precedent already set with operator.
I don't like the fact these are public method, but the only call site is within private methods. Where else do you expect the functions to be called.
The biggest issue is the errors being ignored. If the the env vars can't be parsed into an int, we should raising that error. At the very least we should be logging the error. This should be addressed.
Also can you explain the reasoning for writing variable creation in this style? It would not be a common style in the code base if it is done at all.
var ttl, _ = env.GetInt("DNS_DEFAULT_TTL", builder.DefaultTTL)
// Vs.
ttl, _ := env.GetInt("DNS_DEFAULT_TTL", builder.DefaultTTL)There was a problem hiding this comment.
Got inspired from internal/wasm/utils.go in structure of such getter functions, but I am open to suggestions.
I moved the call for these methods to internal/extension/manager.go reconcile method as that one had access to logger. Not very happy about the "bubbling" of the int values trough method calls, but what do you think?
My mistake about the variable definition, I seemed to forget the idiomatic Golang. Fixed.
| gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" | ||
|
|
||
| kuadrantdnsv1alpha1 "github.com/kuadrant/dns-operator/api/v1alpha1" | ||
| dnsBuilder "github.com/kuadrant/dns-operator/pkg/builder" |
There was a problem hiding this comment.
Can we keep this more consistent? in tests/commons.go it is named as kuadrantdnsbuilder and in the internal/controller/dnspolicy_dnsrecords.go it is not named. I am not asking you to change the existing naming, only that you don't introduce a third name.
There was a problem hiding this comment.
Renamed to kuadrantdnsbuilder, my mistake I did not notice the common naming
azgabur
force-pushed
the
dns_ttl
branch
2 times, most recently
from
f061f0b to
b104be2
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1668 +/- ##
==========================================
- Coverage 75.06% 74.91% -0.15%
==========================================
Files 120 120
Lines 10269 10287 +18
==========================================
- Hits 7708 7707 -1
- Misses 2188 2206 +18
- Partials 373 374 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Not sure how to pass the "Validate release data" test, I even rebased the branch on main trying solving it. Also not sure how to pass the coverage as all of my uncovered statements are logging statements which are not being tested I think. Let me know if I am wrong. Could I get some guidance @Boomatang ? thanks |
|
To get the validate release data it seems like you need to run |
azgabur
force-pushed
the
dns_ttl
branch
2 times, most recently
from
7886135 to
fad206d
Compare
|
Thanks, that worked 👍 |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Signed-off-by: Alex Zgabur <azgabur@redhat.com>
Signed-off-by: Alex Zgabur <azgabur@redhat.com>
Signed-off-by: Alex Zgabur <azgabur@redhat.com>
closes #1628
requires Kuadrant/dns-operator#630
Additionally refactors tests to not use hard coded value but imports constant