-
Notifications
You must be signed in to change notification settings - Fork 332
WPB-27912: Deprecate backgroundEffects feature flag at API v17 #5431
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
Changes from 9 commits
60e6e51
df0faa2
ae0ad03
e021fd1
c968bab
7230bb3
ff1a829
9e21ab5
3fbc0e7
bffb339
4a052bc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| * The `backgroundEffects` team feature flag is **deprecated** (WPB-27912). Its | ||
| default is now **enabled and locked**, and the Helm configuration override for | ||
| `backgroundEffects` has been removed from `charts/wire-server`. The flag's | ||
| data type and its public/internal HTTP endpoints are retained for backward | ||
| compatibility; any Helm overrides for `backgroundEffects` are now ignored and | ||
| can be removed. The public/internal HTTP endpoints return 404 at API version | ||
| v17 and remain available through v16; the flag type remains deprecated. The | ||
| aggregate `GET /feature-configs` and `GET /teams/:tid/features` endpoints | ||
| continue to include `backgroundEffects` at all API versions, including v17. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| The `backgroundEffects` team feature endpoints are deprecated and return 404 for | ||
| clients on API version v17: the public `GET`/`PUT /teams/:tid/features/backgroundEffects` | ||
| and the internal legacy lock `PUT /i/teams/:tid/features/backgroundEffects/(un)?locked`. | ||
| They remain available through v16. The aggregate endpoints | ||
| `GET /feature-configs` and `GET /teams/:tid/features` are unaffected and continue | ||
| to include `backgroundEffects` at all API versions: the aggregate feature list is | ||
| version-agnostic, like other version-gated features such as MLS. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,7 +86,8 @@ type FeatureAPI = | |
| :<|> FeatureAPIGetPut MeetingsConfig | ||
| :<|> Deprecated ::> Until 'V17 ::> FeatureAPIGet MeetingsPremiumConfig | ||
| :<|> Deprecated ::> Until 'V17 ::> FeatureAPIPut MeetingsPremiumConfig | ||
| :<|> FeatureAPIGetPut BackgroundEffectsConfig | ||
| :<|> Deprecated ::> Until 'V17 ::> FeatureAPIGet BackgroundEffectsConfig | ||
| :<|> Deprecated ::> Until 'V17 ::> FeatureAPIPut BackgroundEffectsConfig | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See https://github.com/wireapp/wire-server/pull/5431/changes#r3758084593 - I unfortunately don't understand this split 🤔
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have squashed them |
||
|
|
||
| type VersionedFeatureAPIPut named reqBodyVersion cfg = | ||
| Named | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2429,7 +2429,11 @@ instance ToObjectSchema MeetingsPremiumConfig where | |
| -------------------------------------------------------------------------------- | ||
| -- BackgroundEffects Feature | ||
| -- | ||
| -- Controls whether background effects are available in meetings. | ||
| -- /Deprecated (WPB-27912)./ This feature flag no longer affects meeting | ||
| -- behaviour and is kept solely for API compatibility. It defaults to | ||
| -- /enabled and locked/. Scheduled for removal in a future release. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be Haddock of the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved down |
||
|
|
||
| {-# DEPRECATED BackgroundEffectsConfig "Deprecated (WPB-27912): no longer affects meeting behaviour; kept for API compatibility." #-} | ||
|
|
||
| data BackgroundEffectsConfig = BackgroundEffectsConfig | ||
| deriving (Eq, Show, Generic, GSOP.Generic) | ||
|
|
@@ -2441,7 +2445,7 @@ instance ToSchema BackgroundEffectsConfig where | |
| schema = object objectSchema | ||
|
|
||
| instance Default (LockableFeature BackgroundEffectsConfig) where | ||
| def = defLockedFeature | ||
| def = defLockedFeature {status = FeatureStatusEnabled} | ||
|
|
||
| instance IsFeatureConfig BackgroundEffectsConfig where | ||
| type FeatureSymbol BackgroundEffectsConfig = "backgroundEffects" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,7 +86,8 @@ featureAPI = | |
| <@> featureAPIGetPut @MeetingsConfig | ||
| <@> mkNamedAPI @'("get", MeetingsPremiumConfig) getFeature | ||
| <@> mkNamedAPI @'("put", MeetingsPremiumConfig) setFeature | ||
| <@> featureAPIGetPut @BackgroundEffectsConfig | ||
| <@> mkNamedAPI @'("get", BackgroundEffectsConfig) getFeature | ||
| <@> mkNamedAPI @'("put", BackgroundEffectsConfig) setFeature | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, why do we need this change? Or, what changes it? 🤔
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. outdated |
||
|
|
||
| deprecatedFeatureConfigAPI :: API DeprecatedFeatureAPI GalleyEffects | ||
| deprecatedFeatureConfigAPI = | ||
|
|
||
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.
AFAIK we don't use internal endpoints in a versioned manner. Please check that this doesn't break e.g.
stern.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.
The
Until 'V17on the internal endpoint is safe: internal calls omitX-Wire-API-Version, sogetVersionHeaderdefaults toV0, and the gateV0 >= V17isFalse(never fires). Matches the existingMeetingsPremiumConfigprecedent on the adjacent line; plus, there is no reference instern.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.
@blackheaven Have you considered to completely remove the internal endpoint? Or, who will use the versioned internal endpoint? 🤔