fix(config): fail builds if failed to fetch extensions #6008
Conversation
|
This pull request adds or modifies JavaScript ( |
packages/config/src/api/site_info.ts
Outdated
| throw new Error(`Unexpected status code ${response.status} from fetching extensions`) | ||
| } | ||
| const bodyText = await response.text() | ||
| if (bodyText === '') { |
There was a problem hiding this comment.
Does the endpoint ever return a 200 with an empty body?
There was a problem hiding this comment.
Thanks you for taking a look @eduardoboucas!
We are calling two different endpoints one for team/${accountID}/integrations/installations/meta/${siteID} which gets its response from here in jigsaw.
and site/${siteID}/integrations/safe which gets its response from here in jigsaw.
Both endpoints returns an array for a successful response.
I did it this way because it was a little confusing about how the code was written, specifically this line here about checking if the integrations is an array. On top of that when running tests, a few tests failed since the response body was and empty string and trying to parse it would throw an error.
There were a couple approaches I could have taken:
- by checking if there is a body and returning an empty array.
- updating all tests that failed to have the response body be
[]instead of empty string. I'm not sure if it was intentional that an empty string was returned.
Do you have another suggestion?
Also would it be beneficial use a feature flag here?
There was a problem hiding this comment.
Sorry for the delayed response. My comment was about the fact that if we are hitting endpoints we control, then in theory we should be able to define exactly what the shape of the response body looks like for each supported status code, as opposed to having defensive coding that may obfuscate underlying problems. Like, if a successful response to those endpoints is expected to always return some data, then a 200 with an empty body feels like an unexpected case that we should probably handle as an error.
But it's not a big deal, and you can totally ship this as is! 🚢
There was a problem hiding this comment.
I have a Linear ticket assigned to me to clean up the feature flag after this PR is deployed. I'll update the ticket to include your suggestions, as they make perfect sense and will help avoid confusion in the future.
…//github.com/netlify/build into yj/fail-builds-when-failed-to-fetch-extensions
…tensions from jigsaw
…//github.com/netlify/build into yj/fail-builds-when-failed-to-fetch-extensions
There was a problem hiding this comment.
This looks good to me :) I wish we had a way to test failing the extension fetch when this is deployed just to 100% test it. But the most important piece would be testing if everything works when stuff does not fail
@khendrikse I tested with buildbot using my build image but just to be safe, I'll confirm test again once it has been deployed to production :) |
YujohnNattrass
deleted the
yj/fail-builds-when-failed-to-fetch-extensions
branch
🎉 Thanks for submitting a pull request! 🎉
Summary
A previous PR was shipped however it was reverted when I notice some of my builds were missing integrations.
In the previous PR, we checked the Content-Length header to determine if the response body was empty and returned an empty array in such cases. However, if a site has many extensions, the response will not include the Content-Length header due to chunked encoding. The team I was working with, which has 47 extensions, experienced this issue and was missing the Content-Length header.
Tested build using this build image from this PR
For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)