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

client/fingerprint: add digitalocean fingerprinter #12015

Merged
merged 3 commits into from
Feb 8, 2022

Conversation

kevinschoonover
Copy link
Contributor

adds fingerprinter for DigitalOcean droplets using the DigitalOcean Metadata api.

screenshot of it working on a droplet:
image

return "", err
} else if res.StatusCode != http.StatusOK {
f.logger.Debug("could not read value for attribute", "attribute", attribute, "resp_code", res.StatusCode)
return "", err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error could be nil; its value is unrelated to the status code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this branch lower in the function to include the resp body in the error message. Not 100% sure if this is safe (i.e. will digitalocean always return some body with an error response), but it would improve debugability if it does work

Copy link
Member

@shoenig shoenig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kevinschoonover thanks for the PR! As a DO user myself I'm happy to see this.

I just had a handful of style/convention nitpicks and the one status code err return, if you don't mind taking a look.

Copy link
Member

@shoenig shoenig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(dup)

@vercel vercel bot temporarily deployed to Preview – nomad February 7, 2022 16:54 Inactive
Co-authored-by: Seth Hoenig <[email protected]>
@kevinschoonover
Copy link
Contributor Author

@shoenig thanks for the comments and fast review time! updated the PR to address the concerns. Can you double check my rationale for https://github.com/hashicorp/nomad/pull/12015/files#diff-a213a911b1884870d57540bf67b3dfd2f99f108478e8a930ed42153569e55086R92-R95. I believe digitalocean will return a body with any error message which should make debugging this cleaner, but it may be confusing if the user gets a 500 error and then the code path actually errors out reading the body https://github.com/hashicorp/nomad/pull/12015/files#diff-a213a911b1884870d57540bf67b3dfd2f99f108478e8a930ed42153569e55086R87-R90

Copy link
Member

@shoenig shoenig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; thanks @kevinschoonover !

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
stage/accepted Confirmed, and intend to work on. No timeline committment though. theme/fingerprint type/enhancement
Projects
Development

Successfully merging this pull request may close these issues.

2 participants