Configurable timeouts and synchronisation intervals #117
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kuritka
force-pushed
the
#82-configurable-timeouts-MERGE
branch
2 times, most recently
from
165f226 to
ebba8ad
Compare
ytsarev
requested changes
May 12, 2020
There was a problem hiding this comment.
@kuritka looks very cool. While I'm going through a deeper review, could you please rebase with recent master and make some whitespace busting on the way ?
git rebase master
First, rewinding head to replay your work on top of it...
Applying: Configurable timeouts and synchronisation intervals
Using index info to reconstruct a base tree...
M go.mod
M go.sum
M pkg/controller/gslb/gslb_controller_test.go
.git/rebase-apply/patch:74: new blank line at EOF.
+
.git/rebase-apply/patch:781: new blank line at EOF.
+
.git/rebase-apply/patch:821: new blank line at EOF.
+
.git/rebase-apply/patch:859: new blank line at EOF.
+
.git/rebase-apply/patch:898: new blank line at EOF.
+
warning: squelched 1 whitespace error
warning: 6 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging pkg/controller/gslb/gslb_controller_test.go
Auto-merging go.sum
Auto-merging go.mod
Thanks!
ytsarev
requested changes
May 12, 2020
ytsarev
requested changes
May 12, 2020
kuritka
force-pushed
the
#82-configurable-timeouts-MERGE
branch
2 times, most recently
from
d69f7d6 to
568c0e5
Compare
ytsarev
requested changes
May 13, 2020
kuritka
force-pushed
the
#82-configurable-timeouts-MERGE
branch
from
568c0e5 to
fc3ab54
Compare
ytsarev
previously approved these changes
May 14, 2020
somaritane
suggested changes
May 14, 2020
There was a problem hiding this comment.
@kuritka , looks good to me, just a minor one from me regarding make and GH actions test targets.
resolves #82 remove `pkg/test` (already covered by terratest) and move `helpers.go` from `/pkg/controller/gslb/` to `/pkg/controller/gslb/internal/` add depresolver - abstracts multiple configurations into single point of access - provides predefined values when configuration is missing - validates configuration - executes once covered by unit tests created `internal` to split logic into maintainable packages and reduce public API surface move `YamlToGslb` into `internal utils` (reusing `YamlToGslb` within depresolver tests) Update controller, tests, terratests to be able to use depresolver add configuration for - DnsTtlSeconds - SplitBrainThresholdSeconds - ExternalDNSSyncSeconds - ReconcileRequeueSeconds - _can be extended for other inputs_
kuritka
force-pushed
the
#82-configurable-timeouts-MERGE
branch
from
fc3ab54 to
f927a81
Compare
somaritane
approved these changes
May 14, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #82
remove
pkg/test(already covered by terratest) andhelpers.gofrom/pkg/controller/gslb/add depresolver
covered by unit tests
created
internalto split logic into maintainable packages and reduce public API surfacemove
YamlToGslbintointernal utils(reusing YamlToGslb within depresolver tests)Update controller, tests, terratests to be able to use depresolver
add configuration for