-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/cloud services #422
Conversation
Signed-off-by: Amit Schendel <[email protected]>
Signed-off-by: Amit Schendel <[email protected]>
Signed-off-by: Amit Schendel <[email protected]>
Signed-off-by: Amit Schendel <[email protected]>
Signed-off-by: Amit Schendel <[email protected]>
Signed-off-by: Amit Schendel <[email protected]>
Signed-off-by: Amit Schendel <[email protected]>
Signed-off-by: Amit Schendel <[email protected]>
Signed-off-by: Amit Schendel <[email protected]>
Summary:
|
Signed-off-by: Amit Schendel <[email protected]>
Summary:
|
pkg/dnsmanager/dns_manager.go
Outdated
} | ||
|
||
// Check for other common cloud providers | ||
otherCloudDomains := []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should add ovhcloud.com domain names, since we're partners now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure where to find them...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are some domains in *.ovh.net
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently we only care about aws anyway.
if config.TimeoutSeconds == 0 { | ||
config.TimeoutSeconds = 5 | ||
config.TimeoutSeconds = int(defaultTimeout.Seconds()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks weird to have a duration just to extract an int... tell Claude I would have preferred defaultTimeoutSeconds
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually like this, it's readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah... and it's probably too late to change the config timeoutSeconds into timeoutDuration :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall nice refactoring of the http_exporter
, let's see if we can find OVH domains before merging
Signed-off-by: Amit Schendel <[email protected]>
Signed-off-by: Amit Schendel <[email protected]>
Summary:
|
Signed-off-by: Amit Schendel <[email protected]>
Signed-off-by: Amit Schendel <[email protected]>
Summary:
|
Overview