Add Image Optimizer default settings API #516
Conversation
daboross
force-pushed
the
dross/image-optimizer-default-settings-api
branch
from
c546052 to
3221e40
Compare
|
You'll need to rebase Also, we won't be able to merge this PR until the API documentation is live. |
|
So can you move this PR back to "draft" until API documentation is ready. Just to avoid anyone accidentally merging. Thanks 🙂 |
|
@daboross you'll need to rebase |
|
Thanks for the review! I'll move this back to draft now, and rebase (and probably rebase again before merging!). |
daboross
force-pushed
the
dross/image-optimizer-default-settings-api
branch
from
a6dc69b to
98d89ed
Compare
daboross
force-pushed
the
dross/image-optimizer-default-settings-api
branch
2 times, most recently
from
ef3b647 to
a2e6ecb
Compare
|
I've merged the API documentation PR, now just waiting for that to go live. |
I'd forgotten about this case, but it's used when the user hasn't set any Image Optimizer settings, and it's useful for terraform to be able to easily detect this case. I assume other users of this API will also appreciate this explicit error handling, as opposed to having to check for error 404 themselves.
Previously, ResizeFilter was missing a custom json marshalling function, and any request with a resize filter resulted in a server error. I've expanded testing to test all resize filter values, all jpeg type values, and to include a request which sends all possible keys - just to patch up any other missing untested parts.
daboross
force-pushed
the
dross/image-optimizer-default-settings-api
branch
from
a2e6ecb to
4fc3b72
Compare
|
API docs are live - https://www.fastly.chttps://www.fastly.com/documentation/reference/api/services/image-optimizer-default-settings/! @Integralist Do you have any preference on how this is merged? I'd be happy to squash & merge this now but I want to check in first, since I know you'll be doing the release work & I don't want to presume. |
| ) | ||
|
|
||
| // JpegType is a base for different JpegType variants | ||
| type JpegType int64 |
There was a problem hiding this comment.
This custom type should be prefixed with ImageOpto as it will be globally available across the whole go-fastly package and in the future there might be other features/APIs that have things related to jpegs (maybe?), so we need to be able to distinguish this type as being related to ImageOpto.
EDIT: Let's consider this a general rule across this whole file as I realised there are a few different consts and types that could overlap with future requirements.
|
Right - if the rest of this looks good, should I go ahead and merge this? I think I asked that before but I want to ask again just to confirm 😅 |
|
Just going to give the PR one last look over and then I'll re-approve so you can merge. Thanks! |
|
Thanks for the reviews & all the help here! |
Let me know if there's anything else I need here! I've got some corresponding PRs to other repositories.
I'm a bit unsure if my testing here is appropriate; it's half testing the library, but also half testing the endpoint behaves as I'd expect given the what the library is calling. I figure it should work, but I could also trim it down into a less verbose test.