Skip to content
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

Merged

Conversation

daboross
Copy link
Contributor

@daboross daboross commented Apr 15, 2024

Includes an update to go-fastly 9.4.0.

Copy link
Collaborator

@Integralist Integralist left a 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.

@daboross
Copy link
Contributor Author

@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.

@Integralist
Copy link
Collaborator

👋🏻 @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.

@daboross
Copy link
Contributor Author

daboross commented May 1, 2024

👋

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.

Copy link
Collaborator

@Integralist Integralist left a 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...

daboross added 16 commits May 9, 2024 12:51
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)
@daboross
Copy link
Contributor Author

daboross commented May 9, 2024

Going to rebase to fix merge conflicts, keeping all commits as is otherwise.

@daboross daboross force-pushed the dross/image-optimizer-default-settings-api branch from 4dce077 to b90906b Compare May 9, 2024 19:52
@Integralist Integralist added the enhancement New feature or request label May 10, 2024
Copy link
Collaborator

@Integralist Integralist left a 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.
@daboross daboross marked this pull request as ready for review May 15, 2024 22:40
@daboross
Copy link
Contributor Author

daboross commented May 15, 2024

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.
@Integralist Integralist changed the title Add Image Optimizer default settings API feat(image_optimizer_default_settings): add Image Optimizer default settings API May 16, 2024
@Integralist Integralist merged commit 56868fd into fastly:main May 16, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants