Only checkUpdates if AllowUpdateConnectors enabled

[ambirdsall]

What

Changes the conditional guarding whether we actually query the webBackendCheckUpdates endpoint. The current version checks whether the app is using a cloud build; this updates it to check whether the AllowUpdateConnector feature is enabled, which is the actual source of truth (i.e. if we ever start supporting this endpoint for cloud, that's what would change). Closes https://github.com/airbytehq/airbyte-cloud/issues/3972

on the implementation

The only API we expose for querying FeatureService is a react hook, which can't be directly used outside of other react hooks or components--i.e., not in the ConnectorService class. Conveniently (in the short term, anyway), the only API we expose for using ConnectorService is... a react hook defined in the same file. So the job was reduced to writing an explicit constructor which can be passed the app's feature context via an additional argument. There's a few lines of diff that are just porting the right type definitions for the superclass's constructor args.

I used the FeatureService in a slightly roundabout way, though: for aesthetic reasons, I wanted an object which mapped FeatureItem member keys to boolean values, and useFeatureService doesn't expose anything like that. Maybe I should move the local EnabledFeatures into FeatureService instead, so it's reusable?

[josephkmh]

This is a clever (and very concise) solution!

The approach I was thinking of was to refactor the components that call useGetConnectorsOutOfDate() to avoid calling this hook in the first place. The only way to do this with hooks is to do conditional components. This would require a bit of a larger refactor because of the way useGetConnectorsOutOfDate() is being used currently.

My thoughts for why this is maybe a better approach:

1. We keep our AirbyteRequestService classes very lean, only concerned with data fetching and not other logic (e.g. feature toggling)
2. It's kind of a "lie" to mock out the API response in this way - I can imagine following this pattern could lead to confusion about what's a real API response and what's "intercepted" in the AirbyteRequestService
3. In places where we use useGetConnectorsOutOfDate(), its result is usually being fed to another component that uses a conditional rendering based on useFeature(FeatureItem.AllowUpdateConnectors) (see here). So why not just avoid calling this API endpoint in the first place, if we're avoiding rendering its result later anyway, based on the same feature flag?

[josephkmh]

An alternative to get a list of all enabled features would be:

const { features: enabledFeatures }  = useFeatureService();

Where enabledFeatures: FeatureItem[]. That would avoid needing to manually update this object map here, giving you access to all features as they are added/removed from the provider.

[krishnaglick]

This is a clever (and very concise) solution!

The approach I was thinking of was to refactor the components that call useGetConnectorsOutOfDate() to avoid calling this hook in the first place. The only way to do this with hooks is to do conditional components. This would require a bit of a larger refactor because of the way useGetConnectorsOutOfDate() is being used currently.

My thoughts for why this is maybe a better approach:

1. We keep our `AirbyteRequestService` classes very lean, only concerned with data fetching and not other logic (e.g. feature toggling)

2. It's kind of a "lie" to mock out the API response in this way - I can imagine following this pattern could lead to confusion about what's a real API response and what's "intercepted" in the `AirbyteRequestService`

3. In places where we use `useGetConnectorsOutOfDate()`, its result is usually being fed to another component that uses a conditional rendering based on `useFeature(FeatureItem.AllowUpdateConnectors)` (see [here](https://github.com/airbytehq/airbyte/blob/master/airbyte-webapp/src/pages/SettingsPage/pages/ConnectorsPage/components/ConnectorsView.tsx#L49)). So why not just avoid calling this API endpoint in the first place, if we're avoiding rendering its result later anyway, based on the same feature flag?

While I do agree with some of your points here, I'm not sure it's worth the extra effort for most of it. Primarily because Cloud has a hard-coded response of 0 out of date connectors. The whole permissions issue is separate, but Cloud won't ever have something actionable for the user (in its current state at least).
As long as our feature flagging correctly identifies Cloud vs OSS this solution works fine.

I'm not saying we shouldn't do additional work here, just that I'm not sure it's worth it.

[ambirdsall]

I'm not sure it's the best solution (I'm not sure it isn't either, at least given the current conventions in the codebase; it's at least simple and defensive code), but I think this as-is is better than what we have. I reckon it makes sense to merge this now and then we can figure out if we want to restructure how we do the feature-gating at our leisure.

[krishnaglick]

I'm going to approve this since I feel this is good enough, but as this is in Cloud's Compose's bucket I'll leave it up to y'all if you want to spend more time on this!