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
Replace Update method with field update methods #4
Conversation
Includes: - Improve unit tests
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.
Aside from a few consistency comments, the overall PR looks good.
Expected: 200, | ||
Got: resp.StatusCode, | ||
} | ||
return nil, fmt.Errorf("Unexpected HTTP response. HTTP status code: %d. Body: %s", resp.StatusCode, resp.Body) |
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.
Does this line need an error level like the other errors? (like line 106?)
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.
Good eye! Actually it's the other way around. The convention is to include the log level in calls to log.Printf
and log.Println
, not in the strings used to create error objects, so I will it on line 106.
return nil, &UnexpectedHTTPStatusError{ | ||
Expected: 200, | ||
Got: resp.StatusCode} | ||
return nil, fmt.Errorf("Unexpected HTTP response. HTTP status code: %d. Body: %s", resp.StatusCode, resp.Body) |
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.
Does this need an error level?
Expected: 204, | ||
Got: resp.StatusCode, | ||
} | ||
return fmt.Errorf("Unexpected HTTP response. HTTP status code: %d. Error: %s", resp.StatusCode, err) |
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.
Missing error level?
Expected: 201, | ||
Got: resp.StatusCode, | ||
} | ||
return nil, fmt.Errorf("Unexpected HTTP response. HTTP status code: %d. Body: %s", resp.StatusCode, resp.Body) |
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.
Removed log level for consistency (log level goes on calls to log.Printf
and log.Println
).
This update is to help support the preferred Terraform pattern of updating resource attributes individually rather than several at once.
Includes improved unit tests.
Tests can be run locally, or in a container (see README.md).
When merged into
master
, I'll tag it as v2.0.0, since this breaks the previous public API.