fix(config): fail builds if failed to fetch extensions #6008
Conversation
|
This pull request adds or modifies JavaScript ( |
| 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?
…//github.com/netlify/build into yj/fail-builds-when-failed-to-fetch-extensions
🎉 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)