Add edgedns client #6
Conversation
There was a problem hiding this comment.
Nice. Overall, I think this could be improved by thinking about it from the caller's perspective and making sure it feels natural in Go. That's the main value in client libraries, IMO, and they're not as valuable if they expose HTTP stuff and/or semantics that don't feel right in the target language.
|
|
||
| func (c *Client) ListRecordsets(zone string, queryArgs url.Values) (*RecordsetResponse, error) { | ||
| var rs RecordsetResponse | ||
| err := request.DoJSON(c.Credentials, http.MethodGet, "/config-dns/v2/zones/"+zone+"/recordsets"+handleQueryArgs(queryArgs), nil, &rs) |
There was a problem hiding this comment.
zone should be escaped with url.PathEscape so caller can't corrupt request. Same comment for values inserted into URLs below.
| return "?" + s | ||
| } | ||
|
|
||
| func (c *Client) ListRecordsets(zone string, queryArgs url.Values) (*RecordsetResponse, error) { |
There was a problem hiding this comment.
The goal of the client library is to make the API feel more like the language, so IMO, it's better not to expose HTTP-specific details like url.Values. Also, details of Akamai's protocol could be made to feel more Go-like, and unimportant details could be hidden.
One possible interface, where all but the first argument is optional (can be the zero value):
func (c *Client) GetRecordsets(zone string, recordsetTypes []RecordsetType, sortFields []string, search string) ([]Recordset, error)
If pagination is necessary, maybe it'd be worth implementing a paginator so you could do:
p := edgedns.NewGetRecordsetsPaginator(zone, ...)
for p.HasMorePages() {
page, err := p.NextPage()
...
}
The library would deal with the details of paginating so the client can just do the standard Go thing. (Maybe some common pagination stuff could be added to the requests package.)
There was a problem hiding this comment.
I think I agree with you fundamentally on this if we were trying to implement a general use-case API, but IMO, as far as ADP itself is concerned, since it should be proxying the akamai API and providing relatively minimal interceptions otherwise, I was trying to map a request on an input directly to the same request on the output. If the request came in with args, those args need to map to the backend. I think that's why I started by exposing the url.Values. I might consider using a generic map[string][]string in place of url.Values, instead of breaking it into various variables. The issue that might arise there is if there's a new query arg that gets added and we end up not supporting it properly, and that breaks some functionality down the road.
I like the idea of abstracting those details away, and as long as we can provide functional pass-through of the needed operations, then I'm fully on board with making it Go-like
| return &rs, nil | ||
| } | ||
|
|
||
| func (c *Client) RetrieveRecordsetTypes(zone, name string) (*TypesResponse, error) { |
There was a problem hiding this comment.
IMO, the verb Get is better because it's shorter, more Go-like, and more consistent with the other packages in the module.
There was a problem hiding this comment.
I agree that it is. I think for this case I was looking to the cli for 'guidance' on the naming.
| return &rs, nil | ||
| } | ||
|
|
||
| func (c *Client) RetrieveRecordsetTypes(zone, name string) (*TypesResponse, error) { |
There was a problem hiding this comment.
I don't see a reason to return a struct wrapping the string array here. Could it be the following?
func (c *Client) GetRecordsetTypes(zone, name string) ([]string, error)
|
|
||
| const defaultTTL = 300 | ||
|
|
||
| const ( |
There was a problem hiding this comment.
Would it make sense to make a RecordType enum (type RecordType string and consts for known values)?
| return &rs, nil | ||
| } | ||
|
|
||
| func (c *Client) CreateRecordset(zone, name, recordSetType string, body *Recordset) (*Recordset, error) { |
There was a problem hiding this comment.
Why are the name and recordSetType arguments required if they are also specified in body? What happens if these arguments are in conflict with body? (I realize these are part of the Akamai API, but as a user of this client library, I would be confused. I wonder if we can clarify the API without losing function.)
There was a problem hiding this comment.
I think that in this case, I exposed the direct requirements of the underlying API so we don't end up inadvertently masking a bug or breaking something should they change some undocumented dependency between it being here or in the body. I'm open to making changes, though
PR Description
Implemented into new version of adp, tested successfully
PR Checklist
Examples:
To provide feedback on this template, visit https://docs.google.com/document/d/1YfTv7Amyop5G_8w1c2GJ_Mu-70L0KkZHhm9f9umDi3U/edit