Initial v2 version of GO SDK #5
Conversation
Initial sdk client configuration
Create README.md Stubbing the HttpClient Changing package name for the ci Update README.md Adding CI status Ci (#3)
Added missing tests to the new files Updated the environment resource to match the established format
Cleanup of unused dependencies Fixing build after merge.
skytap/convert.go
Outdated
|
|
||
| // String returns a string value for the passed string pointer. | ||
| // It returns the empty string if the pointer is nil. | ||
| func String(s *string) string { |
There was a problem hiding this comment.
This set of functions seems gratuitous and unnecessary
There was a problem hiding this comment.
These are helper functions because we are using pointers in our structs. We are using pointers so we can support Terraform which may need to know whether a field is initialised or null. Go will assign values to unassigned variables and this might be undesirable. For example: an unassigned int will be given a 0 and Terraform might interpret this as a change where there isn't one.
There are also issues with attaining a pointer to a string literal, e.g., &"string" doesn't work. We have added a set of functions to handle this. We also added functions to go back to a value to provide null checking boiler plate. This provides some symmetry.
We can make them private so they are no exported if you wish?
There was a problem hiding this comment.
@ja6a I understand what they are. please make them private and clean up any naming conflicts.
|
Please run the code through golint or similar. |
|
I notice there is no retry logic in the client, and only DefaultHTTPClient is used. Skytap has specific semantics around resources being busy, and even though a resource may be busy when trying to update, this is not considered a failure. see api/api.go and api/requests.go in the original SDK for context. |
ok we will add golint to the CI process.
The intention is to let the terraform tooling handle these situations. Please refer to : https://www.terraform.io/docs/extend/resources.html |
|
@ja6a and while that may be true, if the SDK is to be used by things other than terraform, the retry logic should be present. |
|
👍 LGTM |
No description provided.