Implement Compute@Edge Free Trial Activation #531
Conversation
| @@ -59,7 +59,7 @@ func main() { | |||
| var ( | |||
| args = os.Args[1:] | |||
| clientFactory = app.FastlyAPIClient | |||
| httpClient = http.DefaultClient | |||
| httpClient = &http.Client{Timeout: time.Second * 5} | |||
There was a problem hiding this comment.
You shouldn't use the golang default http client as it doesn't provide preconfigured timeouts.
For some reason I had previously used a strange pattern of using it with a context object for handling timeouts. The implementation worked but it's a non-standard pattern to use so I've cleaned this up.
| @@ -0,0 +1,57 @@ | |||
| package undocumented | |||
There was a problem hiding this comment.
This is a new package that provides an abstraction for making API calls for undocumented endpoints.
There was a problem hiding this comment.
For whatever reason I'm not overly keen on the name undocumented, even though it's an accurate description of the functionality. So I'm open to suggestions.
I tried using internal prior to this, but the go compiler recognises that name, and will force certain structural constraints that didn't really work for this use case.
| computeBuild := compute.NewBuildCommand(computeCmdRoot.CmdClause, opts.HTTPClient, globals, data) | ||
| computeDeploy := compute.NewDeployCommand(computeCmdRoot.CmdClause, opts.HTTPClient, globals, data) | ||
| computeInit := compute.NewInitCommand(computeCmdRoot.CmdClause, opts.HTTPClient, globals, data) | ||
| computeBuild := compute.NewBuildCommand(computeCmdRoot.CmdClause, globals, data) |
There was a problem hiding this comment.
A bunch of commands were explicitly passing in a HTTP client and then not using it.
I additionally (see below in pkg/app/run.go) decided to embed the HTTP client into the globals object so it wouldn't be necessary to have to pass it explicitly into the remaining commands, as we already are passing in the global object.
| @@ -148,7 +149,7 @@ func Run(opts RunOpts) error { | |||
| } | |||
| } | |||
|
|
|||
| globals.Client, err = opts.APIClient(token, endpoint) | |||
| globals.APIClient, err = opts.APIClient(token, endpoint) | |||
There was a problem hiding this comment.
The use of Client was becoming ambiguous due to us passing around both an API client and a standard HTTP client, so I made it clearer by renaming the field across the code base. Most of the file churn comes from this change.
| @@ -80,6 +80,12 @@ func listDomainsOk(i *fastly.ListDomainsInput) ([]*fastly.Domain, error) { | |||
| }, nil | |||
| } | |||
|
|
|||
| func getServiceDetailsWasm(i *fastly.GetServiceInput) (*fastly.ServiceDetail, error) { | |||
There was a problem hiding this comment.
Now we have an additional validation step that calls the 'Service Details' API endpoint, we need a mock response for our test coverage.
|
|
||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, configEndpoint, nil) | ||
| func (f *File) Load(endpoint, path string, c api.HTTPClient) error { | ||
| req, err := http.NewRequest(http.MethodGet, endpoint, nil) |
There was a problem hiding this comment.
We no longer require a complex request object (that accepts a context), as the client itself has a timeout configured.
| @@ -11,3 +13,21 @@ func APIClient(a API) func(string, string) (api.Interface, error) { | |||
| return a, nil | |||
| } | |||
| } | |||
|
|
|||
| type mockHTTPClient struct { | |||
There was a problem hiding this comment.
Mock HTTP client for the sake of validating the undocumented 'activate trial' API.
| CloneVersionFn: testutil.CloneVersionError, | ||
| ListVersionsFn: testutil.ListVersions, | ||
| CloneVersionFn: testutil.CloneVersionError, | ||
| GetServiceDetailsFn: getServiceDetailsWasm, |
There was a problem hiding this comment.
There were quite a few existing tests that needed to be fixed so they mocked GetServiceDetailsFn, which was a new API call I added for validating the user's service type.
| @@ -225,6 +232,98 @@ func TestDeploy(t *testing.T) { | |||
| "Creating service...", | |||
| }, | |||
| }, | |||
| // The following test mocks the service creation to fail with a specific | |||
There was a problem hiding this comment.
Here are a bunch of new tests that validate different failure and success scenarios related to the trial activation.
| @@ -624,6 +684,28 @@ func manageExistingServiceFlow( | |||
| return serviceVersion, err | |||
| } | |||
|
|
|||
| // Validate that we're dealing with a Compute@Edge 'wasm' service and not a | |||
There was a problem hiding this comment.
I realised when dealing with a user who provides their own service, that we never actually validate the service is a compatible wasm service type. So I added that validation step here...
Integralist
requested review from
a team and
kailan
and removed request for
a team
There's two flows when it comes to the C@E free trial activation…
For the first scenario, if the service fails to be created, then we attempt to activate the trial and recall the function that creates the service.
For the second scenario I've added a validation step to check if the given service ID is associated with a Wasm service (e.g. I call the service details API endpoint and check the ‘type’ field is set to ‘wasm’). If that fails, then it means the user provided a VCL Service. The error we return indicates we need a Wasm service, not a VCL service and the user should consult fastly.help/cli/ecp-feature.
NOTE: This PR includes code changes not directly related to the free trial activation but helped to clean the code up.