Add dynamic servers support

[asobrien]

This adds support for the API's dynamic servers to the client via the Pool and Server resources.

[asobrien]

The fixtures for the pool and server resources are currently absent--I wasn't sure what the process around generating them looks like.

[phamann]

Hi @asobrien, thank you for taking the time to add this contribution to the library, we really appreciate it.

There are only a few comments/changes I've requested, mainly to increase consistency across the library, and a comment about update pointers which it would be good to get your opinion on.

With regards to testing, you'll need to run make test with a valid API token, which will execute the requests against the API and record the results (with the API token redacted) for subsequent runs. Also take a look at the helpers @devkitx added in his version of this in #143, specifically here: https://github.com/fastly/go-fastly/pull/143/files#diff-997cff4762ab0b7c60bcaf50fcc36d6e. Hope that helps.

Let us know if you have any further questions.

[asobrien]

Thanks for the comments!

With regards to testing, you'll need to run make test with a valid API token, which will execute the requests against the API and record the results (with the API token redacted) for subsequent runs.

I don't have API credentials to access to the baked-in test service. Do maintainers generally run the tests against this service to generate the fixtures? Or, alternatively should a contributor run the test against a Fastly service they have access to and then update the fixtures accordingly? I'm just trying to get a handle on the process around this.

The only issue with running the tests against an "unofficial" service, is that the version number recorded in the responses become somewhat arbitrary. Just noting that if this is the recommended route, it may be worth allowing the test service ID to be defined via an env var and adding a recorder Filter that modifies the request/response to keep a consistent service ID.

Also take a look at the helpers @devkitx added in his version of this in #143, specifically here: https://github.com/fastly/go-fastly/pull/143/files#diff-997cff4762ab0b7c60bcaf50fcc36d6e.

I've included nearly identical helpers. I don't have a baked in testPoolID as Pools are created as needed via the createTestPool helper. Not referencing a hard-coded testServiceID also sidesteps the issue of the tests not being portable between services as the Pool is tied to a specific service. Maybe I'm missing something?

[asobrien]

A few clarification questions.

[asobrien]

@phamann I've updated this PR to use pointers for optional fields in structs (see
cf93538). I limited the use of pointers to optional fields as this seemed like a good compromise between the existing library and the required functionality.

It would be possible for all the fields in the input structs to be pointers, but this requires adding new nil pointer checks (e.g., here) that differs from the rest of the library. These value fields are also required in each input struct, so there's no real need from them to be pointers. I did modify the Pool update test case to ensure we can actually set a "zero" value (see here and here)

A didn't modify the response structs to use pointer fields either. My thoughts around this are: keeping these as value fields keeps the response consistent with the rest of the library (no need to start dereferencing the reponses). Moreover, as far as I can tell, the Fastly API always populates all the fields in the response which means that we don't need to rely on pointer fields to omit missing fields.

I added some helper functions (cf. 2767792) to make it easier for a user to set optional fields. I also generated the fixtures (and replaced the test service id to match that baked into the test client).

Let me know what you think about how optional fields are being handled in the Pool and Server resources and if that's what you had in mind.

[phamann]

👍 Looking good to me now.

Next steps:

• I want @philippschulte to have a quick review
• With regards to only using pointers in the optional fields. I'm going to ask the opinion of some others, as whilst I like it, its inconsistent with another private branch we have which is also introducing pointers for update inputs.

Hopefully we can get the above approved and merged in the next couple of days. Once again thank you for all of the work you've put into this and your patience.

[philippschulte]

@asobrien thanks for your contribution! Using pointers does solve the zero value issue. However, since the library uses github.com/ajg/form" to encode the request struct I am wondering if we can avoid pointers. The package does provide a KeepZeros function which the go-fastly library is already using. I have already tried to fix this issue a couple of months ago but haven't really had the time to dig into it. @phamann I have already asked for an opinion on this issue internally. Please have a look here.

There must be a way to avoid using pointers. If we decide not to find an alternative then I am fine with using pointers and the PR looks good to me. The convenience functions for creating pointer types are useful and the go-github client provides them too.

[philippschulte]

@asobrien thanks for addressing the PR comments but I can't find 59ec1eb, 816dd2b, and 25093b6. Where did they go 🤔

[asobrien]

Sorrty @philippschulte forgot to push before I got distracted. Should be there now.

[philippschulte]

@asobrien looks good but also please update UseTLS and TLSCheckCert to * Compatibool in the response struct too. After that, I can merge and cut a new release.

[asobrien]

@philippschulte I'm not sure there's a way to get rid of pointers in the input. Looking into the KeepZeros option in github.com/ajg/form, it looks that option ensure that zero-values will be submitted as a native-type (desired behavior) rather than an a empty string. Without using pointers for optional inputs, it's impossible to set/revert/update and field to be represented by the zero-value as this will get dropped during marshalling as a result of the omitempty tag. I tested this behavior on e088bc5 and it isn't possible to set Quorum = 0, for example.

Getting rid of the omitempty tag allows the zero-values to be set, but every update will overwrite any fields that aren't explicitly defined--I'm not sure this is desirable either. Pointers the only way I know of that allows for optional fields to be optional and for zero-values to be used.

@phamann I looked into the API response (for the Pool resource) in a little more depth, and it seems like the Fastly API doesn't explicitly set unset fields to the default value but rather sets them to null, for example:

$ curl -sS -H "Fastly-Key: ${FASTLY_API_KEY}" -X GET "https://api.fastly.com/service/deadbeef/version/48/pool" | jq
[
  {
    "name": "mypool",
    "first_byte_timeout": "15000",
    "tls_ciphers": null,
    "type": "random",
    "healthcheck": null,
    "max_conn_default": "100",
    "service_id": "deadbeef",
    "tls_check_cert": "0",
    "created_at": "2020-01-29T13:57:10Z",
    "quorum": "0",
    "request_condition": null,
    "tls_sni_hostname": null,
    "deleted_at": null,
    "min_tls_version": null,
    "shield": null,
    "tls_cert_hostname": null,
    "between_bytes_timeout": "10000",
    "version": "48",
    "max_tls_version": null,
    "use_tls": "0",
    "tls_ca_cert": null,
    "updated_at": "2020-01-29T18:55:20Z",
    "id": "abcdef0123456789",
    "tls_client_cert": null,
    "connect_timeout": "1000",
    "tls_client_key": null,
    "comment": "",
    "override_host": null
  }
]

without using pointers in the response struct, it's not possible to tell whether those fields are unset ("shield": null) or are just set the default value ("shield": "").

[asobrien]

please update UseTLS and TLSCheckCert to * Compatibool in the response struct too

Fixed in 621dd64

[philippschulte]

@asobrien that is exactly what I found out about the KeepZeros option too 😆. I appreciate that you also looked into it because I was thinking I am not using it correctly. @phamann told me this morning that he is fine whit the pointer solution and that it will be the new standard of the library!

For consistency please use a pointer for Compatibool in the response struct (621dd64). I am happy to merge the PR and cut a new release after this has been addressed. Thanks

[asobrien]

@philippschulte Will do! I went through the library, and it looks like none of the response structures currently use a Compatibool type (they all use bool), for example in logentries, syslog and backend. I can confirm the response correctly unmarshals into a bool.

In this race, I've got no horse. I just wanted to point it out since it makes a break with the existing response structs.

[philippschulte]

@asobrien you're absolutely right! Too many structs 😄. Sorry about the confusion! Thanks again for all the hard work you put into this PR!

[asobrien]

No problem. Just to clarify: we should use a bool in the Pool response struct?

If yes, let me revert and clean up the last commit and then we should be good to go.

[philippschulte]

@asobrien yes please revert it! My fault, sorry 😭

[asobrien]

@philippschulte fixed in 0f6b649

[philippschulte]

@asobrien v1.5.0 has been released! Thanks again.