Skip to content
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

Merged
merged 14 commits into from
Dec 2, 2024
Merged

Feature/cloud services #422

merged 14 commits into from
Dec 2, 2024

Conversation

amitschendel
Copy link
Collaborator

Overview

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]>
Copy link

github-actions bot commented Dec 1, 2024

Summary:

  • License scan: success
  • Credentials scan: failure
  • Vulnerabilities scan: failure
  • Unit test: success
  • Go linting: success

Copy link

github-actions bot commented Dec 1, 2024

Summary:

  • License scan: success
  • Credentials scan: failure
  • Vulnerabilities scan: failure
  • Unit test: success
  • Go linting: success

}

// Check for other common cloud providers
otherCloudDomains := []string{
Copy link
Contributor

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

Copy link
Contributor

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...

Copy link
Contributor

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

Copy link
Collaborator Author

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())
Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Contributor

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 :(

Copy link
Contributor

@matthyx matthyx left a 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]>
matthyx
matthyx previously approved these changes Dec 2, 2024
Copy link

github-actions bot commented Dec 2, 2024

Summary:

  • License scan: success
  • Credentials scan: failure
  • Vulnerabilities scan: failure
  • Unit test: success
  • Go linting: success

@amitschendel amitschendel added release Create release and removed release Create release labels Dec 2, 2024
Copy link

github-actions bot commented Dec 2, 2024

Summary:

  • License scan: failure
  • Credentials scan: failure
  • Vulnerabilities scan: failure
  • Unit test: success
  • Go linting: success

@amitschendel amitschendel merged commit b4297f3 into main Dec 2, 2024
19 checks passed
@amitschendel amitschendel deleted the feature/cloud-services branch December 2, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants