feat(contact): add support for customer contacts#813
Conversation
Adds Client.ListContacts, Client.CreateContact, and Client.DeleteContact
backed by /customer/{customer_id}/contacts. The endpoint uses JSON:API
with the customer_contact resource type. The Fastly API does not expose
an update endpoint for contacts, so only list/create/delete are
implemented.
Includes VCR fixtures and validation tests for missing required inputs.
|
Thanks for the PR! Unfortunately we are no longer adding API endpoints to the top-level |
|
Thanks for the quick review and for pointing me to the right pattern! I'll move the new endpoints into a dedicated |
Per review feedback on fastly#813, move the customer contacts API (list/create/delete) out of the root fastly package and into fastly/customer/contacts/, matching the convention established by fastly/apisecurity/operations. - Rename ListContacts/CreateContact/DeleteContact to contacts.{List,Create,Delete} with (ctx, *fastly.Client, *Input) signatures and pointer-valued input fields. - Keep JSON:API marshaling via Client.PostJSONAPI and the google/jsonapi unmarshaling helpers; preserve the internal payload struct that omits CustomerID from the body. - Move fixtures to fastly/customer/contacts/fixtures/ (API paths and cassette contents unchanged). - Update CHANGELOG entry scope to customer/contacts.
|
Done — I've pushed the refactor. The contacts client now lives at |
|
Thanks - we're about to start a three day weekend, so we'll look into this next week. |
kpfleming
left a comment
kpfleming
left a comment
There was a problem hiding this comment.
Some small change requests, but overall this look great!
| func deref(s *string) string { | ||
| if s == nil { | ||
| return "" | ||
| } | ||
| return *s | ||
| } |
There was a problem hiding this comment.
I believe ToValue() already exists for this purpose.
There was a problem hiding this comment.
Thanks, I'd missed that one. Updated to use fastly.ToValue and dropped the local helper.
| fastly.Record(t, "get_current_user", func(c *fastly.Client) { | ||
| var u *fastly.User | ||
| u, err = c.GetCurrentUser(ctx) | ||
| if err == nil && u != nil && u.CustomerID != nil { | ||
| customerID = *u.CustomerID | ||
| } | ||
| }) | ||
| require.NoError(t, err) | ||
| require.NotEmpty(t, customerID) |
There was a problem hiding this comment.
This does not need to be recorded as a fixture, it's an API call which is not part of the package being tested.
There was a problem hiding this comment.
Good point. Removed the GetCurrentUser call and its fixture. I now source the customer ID from a hardcoded constant in the test, the same way apisecurity/operations uses fastly.TestDeliveryServiceID for its service ID.
GetCurrentUser is an API call outside the package being tested, so it should not be recorded as a fixture. Use a hardcoded customer ID instead, matching the apisecurity/operations pattern.
|
|
||
| // The customer ID is an opaque identifier that is only replayed against the | ||
| // recorded fixtures; its exact value does not matter for the test. | ||
| customerID := "7i6ZbMEdjnTUNyk75XgWO0" |
There was a problem hiding this comment.
This should probably use the GetCurrentCustomer API endpoint (https://www.fastly.com/documentation/reference/api/account/customer/#get-logged-in-customer), but without recording it into the fixtures. This can be done by using DefaultClient() to get a regular Fastly client, avoiding the recording process.
There was a problem hiding this comment.
Sorry for the delay, busy week. GetCurrentCustomer isn't implemented in go-fastly today, so i'm not sure i follow what you mean here. I hardcoded the customer id because that's how it's done in the other tests (token_test.go, account_event_test.go), so i followed the same pattern.
If you'd rather fetch it at runtime, the closest thing i see is to grab a regular client with DefaultClient() and call GetCurrentUser, the returned User have a CustomerID field, something like:
user, err := fastly.DefaultClient().GetCurrentUser(ctx)
require.NoError(t, err)
customerID := fastly.ToValue(user.CustomerID)Let me know which you prefer.
There was a problem hiding this comment.
The underlying issue is that hardcoding a CID won't work, because it will have to match the CID linked to the token that is being used to authenticate to the API or the tests will fail. If we followed the existing pattern there would have to be additions to fastly_test_utils.go to hold the hardcoded CID, and every developer trying to run this test would have to provide the CID which matches the token they are using.
However, since these are the only API calls/tests which require a CID, I would be fine with determining that CID at runtime so that running the tests doesn't require additional setup. While GetCurrentUser can be used for that, we've also seen a number of cases where that API operation returns an error because the API token is service-limited or has some other limits on it, but GetCurrentCustomer should not fail in those cases. That's why I suggested using GetCurrentCustomer.
We can implement GetCurrentCustomer if that's something you'd prefer us to do. it would be implemented in the fastly package since it's a client-related operation and does not need to be in a subpackage.
3 participants
Change summary
Adds support for the customer contacts API under
/customer/{customer_id}/contacts.Three new client methods:
Client.ListContactsClient.CreateContactClient.DeleteContactThe endpoint uses JSON:API with the
customer_contactresource type. The Fastly API does not expose an update endpoint for contacts, so only list/create/delete are implemented.The integration test creates a guard contact first because the API refuses to delete the last contact of a given type (
You cannot remove the last <type> contact).All Submissions:
New Feature Submissions:
User Impact
Users can now manage customer-level contacts (primary, billing, technical, security, emergency) through the Go client instead of having to call the API directly. This unblocks support for customer contacts in the Terraform provider.
Are there any considerations that need to be addressed for release?
No breaking changes. New surface only.