-
Notifications
You must be signed in to change notification settings - Fork 78
fix!: remove addons #6193
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
fix!: remove addons #6193
Conversation
db5882a
to
2354a7d
Compare
2354a7d
to
6e9b9d5
Compare
This pull request adds or modifies JavaScript ( |
6e9b9d5
to
b6de27c
Compare
This pull request adds or modifies JavaScript ( |
b6de27c
to
a6770d0
Compare
This pull request adds or modifies JavaScript ( |
_Type_: `NetlifyClient?` | ||
|
||
Netlify [JavaScript client instance](https://github.com/netlify/js-client) used to retrieve [`siteInfo`](#siteinfo), | ||
[`accounts`](#accounts) and [`addons`](#addons). |
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.
I did a humio search for path="/api/v1/sites/*/service-instances"
and have a handful of requests with ua=netlify/js-client
. Are they not coming from this repository? Do you have a sense of where they are coming from?
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.
There are other consumers of the netlify
package, so it's hard to say. Certainly at least some of those requests are originating from this repository (I'm removing the code that performs these requests), but I think the more salient question is: Is anyone using the data returned from this query? (Eduardo seems to think no (internal link))
If anyone actually is relying on this data, I've laid out the migration path. (I'd also be happy to help contribute changes if someone can tell me where we're using this data.) But as far as I've been able to figure out, this is just vestigial code.
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
Currently, we conditionally attempt to retrieve information about addons in `@netlify/config#resolveConfig`. As far as I can tell, every query to the `listServiceInstancesForSite` (addon info) Bitballoon endpoint fails with a 500 and the following response body: ```json { error: '{"message":"Missing Authentication Token"}' } ``` ...this, despite verifying that e.g. the accounts and site info queries right next to it use the exact same API token and succeed. Unfortunately, because of the way we do error handling, when this query fails it fails all of the surrounding queries. We could change this code path to have separate `try/catch`es for each query, but the CLI (and probably other consumers) currently assume this behavior, so that would be an obtrusive change. Addons are a thing of the past and we're fairly sure no one is using them any longer. (The CLI is certainly not.) Any consumers of this module who still rely on this information and for whom this query somehow _does_ work can instead perform this query in application code. I started out just removing this from the `@netlify/config` module, but decided to remove addons entirely from this repo as to not leave this confusing vestigial concept around.
a6770d0
to
13405df
Compare
This pull request adds or modifies JavaScript ( |
Currently, we conditionally attempt to retrieve information about addons in
@netlify/config#resolveConfig
. As far as I can tell, every query to thelistServiceInstancesForSite
(addon info) Bitballoon endpoint fails with a 500 and the following response body:...this, despite verifying that e.g. the accounts and site info queries right next to it use the exact same API token and succeed.
Unfortunately, because of the way we do error handling, when this query fails it fails all of the surrounding queries. We could change this code path to have separate
try/catch
es for each query, but the CLI (and probably other consumers) currently assume this behavior, so that would be an obtrusive change.Addons are a thing of the past and we're fairly sure no one is using them any longer. (The CLI is certainly not.) Any consumers of this module who still rely on this information and for whom this query somehow does work can instead perform this query in application code.
I started out just removing this from the
@netlify/config
module, but decided to remove addons entirely from this repo as to not leave this confusing vestigial concept around.