-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(image_optimizer_default_settings): add Image Optimizer default settings API #832
feat(image_optimizer_default_settings): add Image Optimizer default settings API #832
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small comment for the moment, but it's looking good. Let me know when you're ready for a full review.
fastly/block_fastly_service_image_optimizer_default_settings.go
Outdated
Show resolved
Hide resolved
@Integralist I'd request a full review whenever you've got the time! I'll leave this as a draft until the api-documentation & go-fastly PRs are merged, but I've finally gotten this working. Also, my apologies, I made some modifications to the go-fastly PR to better test it & better support the terraform use case. Probably should have waited until I had everything working from one end to the other to submit PRs, but well you live & learn. I'll clean up / squash the commit messages - they're a bit of a mess - but the code itself should be good. |
👋🏻 @daboross apologies I've been stacked up with other work and I didn't realise it's been 2 weeks since you last commented. So I'll try to take a look at this PR tomorrow morning for you. |
👋 It's no problem! I also could have commented something: after finalizing the api-documentation PR, I went and cleaned up the go-fastly PR, and was starting to clean up this one up when I got sidetracked by a missing consideration in the original API. I'll push those commits so tomorrow morning you can check it out, and leave a comment where I have one part untested because of a change needed to the API itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one comment re: tests...
fastly/block_fastly_service_image_optimizer_default_settings_test.go
Outdated
Show resolved
Hide resolved
I was initially trying to get Terraform to ignore settings that wern't set in the configuration, leaving them at their previously configured value. This seems to be counter to Terraform's design, though, so I've relented & included the current default values for each setting here. It seems like maybe we could have done that for the string values, and non-0 integers, but it seems like a nonstarter for booleans (hashicorp/terraform-plugin-sdk#817), and I didn't want to go against Terraform's design too much.
Part of this is updating field with descriptions from the final api-documentation PR. The other part is just my messing with stuff to try to make it look better.
(also, move image_optimizer_default_settings guide from docs to templates)
Going to rebase to fix merge conflicts, keeping all commits as is otherwise. |
4dce077
to
b90906b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Mark the PR 'ready for review' and I'll give it a final going over.
…tion. This is working well! Just wanted to add this to double-confirm it's doing what I expect.
This should be ready for a full review! I've included an update to the released go-fastly 9.4.0. |
Not sure why updating had caused this, but this should still work.
Includes an update to go-fastly 9.4.0.