From a2e40f1e5a5238d93d325c3877ae7e521f39f8d1 Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Fri, 13 Sep 2024 15:05:00 -0700 Subject: [PATCH 1/7] chore: move SLACK_ENABLE_AVATARS from config to feature flag Needed to make SLACK_ENABLE_AVATARS more dynamic @ Preset so it's easy for us to turn this feature on/off and allow for a progressive rollout. --- UPDATING.md | 1 + superset-frontend/src/components/FacePile/index.tsx | 7 ++----- superset/config.py | 9 ++++----- superset/views/base.py | 1 - superset/views/users/api.py | 6 +++--- tests/integration_tests/users/api_tests.py | 5 +++-- 6 files changed, 13 insertions(+), 16 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 1d22d2be17d05..238b6d06f8977 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -60,6 +60,7 @@ assists people when migrating to a new version. - [29264](https://github.com/apache/superset/pull/29264) Slack has updated its file upload api, and we are now supporting this new api in Superset, although the Slack api is not backward compatible. The original Slack integration is deprecated and we will require a new Slack scope `channels:read` to be added to Slack workspaces in order to use this new api. In an upcoming release, we will make this new Slack scope mandatory and remove the old Slack functionality. - [29798](https://github.com/apache/superset/pull/29798) Since 3.1.0, the intial schedule for an alert or report was mistakenly offset by the specified timezone's relation to UTC. The initial schedule should now begin at the correct time. - [30021](https://github.com/apache/superset/pull/30021) The `dev` layer in our Dockerfile no long includes firefox binaries, only Chromium to reduce bloat/docker-build-time +- []() Moved SLACK_ENABLE_AVATAR from config.py to the feature flag framework, please adapt your configs ### Potential Downtime diff --git a/superset-frontend/src/components/FacePile/index.tsx b/superset-frontend/src/components/FacePile/index.tsx index 51e44d248e6cb..ae5a53d6870db 100644 --- a/superset-frontend/src/components/FacePile/index.tsx +++ b/superset-frontend/src/components/FacePile/index.tsx @@ -16,15 +16,14 @@ * specific language governing permissions and limitations * under the License. */ -import { useSelector } from 'react-redux'; import { getCategoricalSchemeRegistry, styled, + isFeatureEnabled, SupersetTheme, } from '@superset-ui/core'; import { Tooltip } from 'src/components/Tooltip'; import { Avatar } from 'src/components'; -import { RootState } from 'src/views/store'; import { getRandomColor } from './utils'; interface FacePileProps { @@ -55,9 +54,7 @@ const StyledGroup = styled(Avatar.Group)` `; export default function FacePile({ users, maxCount = 4 }: FacePileProps) { - const enableAvatars = useSelector( - state => state.common?.conf?.SLACK_ENABLE_AVATARS, - ); + const enableAvatars = isFeatureEnabled('SLACK_ENABLE_AVATARS'); return ( {users.map(({ first_name, last_name, id }) => { diff --git a/superset/config.py b/superset/config.py index d43028fe64410..79b3f2de9918b 100644 --- a/superset/config.py +++ b/superset/config.py @@ -545,6 +545,10 @@ class D3TimeFormat(TypedDict, total=False): "SQLLAB_FORCE_RUN_ASYNC": False, # Set to True to to enable factory resent CLI command "ENABLE_FACTORY_RESET_COMMAND": False, + # Whether Superset should use Slack avatars for users. + # If on, you'll want to add "https://avatars.slack-edge.com" to the list of allowed + # domains in your TALISMAN_CONFIG + "SLACK_ENABLE_AVATARS": False, } # ------------------------------ @@ -1427,11 +1431,6 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument SLACK_API_TOKEN: Callable[[], str] | str | None = None SLACK_PROXY = None -# Whether Superset should use Slack avatars for users. -# If on, you'll want to add "https://avatars.slack-edge.com" to the list of allowed -# domains in your TALISMAN_CONFIG -SLACK_ENABLE_AVATARS = False - # The webdriver to use for generating reports. Use one of the following # firefox # Requires: geckodriver and firefox installations diff --git a/superset/views/base.py b/superset/views/base.py index adfb8b3db5700..33ba43b2f6870 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -114,7 +114,6 @@ "NATIVE_FILTER_DEFAULT_ROW_LIMIT", "PREVENT_UNSAFE_DEFAULT_URLS_ON_DATASET", "JWT_ACCESS_CSRF_COOKIE_NAME", - "SLACK_ENABLE_AVATARS", "SQLLAB_QUERY_RESULT_TIMEOUT", ) diff --git a/superset/views/users/api.py b/superset/views/users/api.py index a7000b6b96c00..8d07ee29b9a7e 100644 --- a/superset/views/users/api.py +++ b/superset/views/users/api.py @@ -21,6 +21,7 @@ from superset import app from superset.daos.user import UserDAO +from superset.extensions import feature_flag_manager from superset.utils.slack import get_user_avatar, SlackClientError from superset.views.base_api import BaseSupersetApi from superset.views.users.schemas import UserResponseSchema @@ -143,10 +144,9 @@ def avatar(self, user_id: int) -> Response: # fetch from the one-to-one relationship if len(user.extra_attributes) > 0: avatar_url = user.extra_attributes[0].avatar_url - should_fetch_slack_avatar = app.config.get( - "SLACK_ENABLE_AVATARS" - ) and app.config.get("SLACK_API_TOKEN") + "SLACK_API_TOKEN" + ) and feature_flag_manager.is_feature_enabled("SLACK_ENABLE_AVATARS") if not avatar_url and should_fetch_slack_avatar: try: # Fetching the avatar url from slack diff --git a/tests/integration_tests/users/api_tests.py b/tests/integration_tests/users/api_tests.py index 4153a5bd08fb0..7894b1856c1ef 100644 --- a/tests/integration_tests/users/api_tests.py +++ b/tests/integration_tests/users/api_tests.py @@ -22,7 +22,7 @@ from superset import security_manager from superset.utils import json, slack # noqa: F401 from tests.integration_tests.base_tests import SupersetTestCase -from tests.integration_tests.conftest import with_config +from tests.integration_tests.conftest import with_config, with_feature_flags from tests.integration_tests.constants import ADMIN_USERNAME meUri = "/api/v1/me/" @@ -81,7 +81,8 @@ def test_avatar_valid_user_no_avatar(self): response = self.client.get("/api/v1/user/1/avatar.png", follow_redirects=False) assert response.status_code == 204 - @with_config({"SLACK_API_TOKEN": "dummy", "SLACK_ENABLE_AVATARS": True}) + @with_config({"SLACK_API_TOKEN": "dummy"}) + @with_feature_flags(SLACK_ENABLE_AVATARS=True) @patch("superset.views.users.api.get_user_avatar", return_value=AVATAR_URL) def test_avatar_with_valid_user(self, mock): self.login(ADMIN_USERNAME) From a3f196fc38416ac2c2e1bb2e2a3beb60f54d960c Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Fri, 13 Sep 2024 15:18:53 -0700 Subject: [PATCH 2/7] link in UPDATING.md --- UPDATING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/UPDATING.md b/UPDATING.md index 238b6d06f8977..412bff8596866 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -60,7 +60,7 @@ assists people when migrating to a new version. - [29264](https://github.com/apache/superset/pull/29264) Slack has updated its file upload api, and we are now supporting this new api in Superset, although the Slack api is not backward compatible. The original Slack integration is deprecated and we will require a new Slack scope `channels:read` to be added to Slack workspaces in order to use this new api. In an upcoming release, we will make this new Slack scope mandatory and remove the old Slack functionality. - [29798](https://github.com/apache/superset/pull/29798) Since 3.1.0, the intial schedule for an alert or report was mistakenly offset by the specified timezone's relation to UTC. The initial schedule should now begin at the correct time. - [30021](https://github.com/apache/superset/pull/30021) The `dev` layer in our Dockerfile no long includes firefox binaries, only Chromium to reduce bloat/docker-build-time -- []() Moved SLACK_ENABLE_AVATAR from config.py to the feature flag framework, please adapt your configs +- [30274](https://github.com/apache/superset/pull/30274) Moved SLACK_ENABLE_AVATAR from config.py to the feature flag framework, please adapt your configs ### Potential Downtime From 6dad02275de7e1e7ac716770554eb388fc22f9f4 Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Fri, 13 Sep 2024 15:24:33 -0700 Subject: [PATCH 3/7] micro optimization --- superset/views/users/api.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/superset/views/users/api.py b/superset/views/users/api.py index 8d07ee29b9a7e..82089fe84fe20 100644 --- a/superset/views/users/api.py +++ b/superset/views/users/api.py @@ -19,9 +19,8 @@ from flask_jwt_extended.exceptions import NoAuthorizationError from sqlalchemy.orm.exc import NoResultFound -from superset import app +from superset import app, is_feature_enabled from superset.daos.user import UserDAO -from superset.extensions import feature_flag_manager from superset.utils.slack import get_user_avatar, SlackClientError from superset.views.base_api import BaseSupersetApi from superset.views.users.schemas import UserResponseSchema @@ -144,10 +143,12 @@ def avatar(self, user_id: int) -> Response: # fetch from the one-to-one relationship if len(user.extra_attributes) > 0: avatar_url = user.extra_attributes[0].avatar_url - should_fetch_slack_avatar = app.config.get( - "SLACK_API_TOKEN" - ) and feature_flag_manager.is_feature_enabled("SLACK_ENABLE_AVATARS") - if not avatar_url and should_fetch_slack_avatar: + slack_token = app.config.get("SLACK_API_TOKEN") + if ( + not avatar_url + and slack_token + and is_feature_enabled("SLACK_ENABLE_AVATARS") + ): try: # Fetching the avatar url from slack avatar_url = get_user_avatar(user.email) From 842e9aeb6a55af6698ab0764ccdac0ce31d83305 Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Fri, 13 Sep 2024 16:01:53 -0700 Subject: [PATCH 4/7] fixin' --- .../packages/superset-ui-core/src/utils/featureFlags.ts | 1 + superset-frontend/src/components/FacePile/index.tsx | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts b/superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts index 67f3785ab60a1..8801706c55ff6 100644 --- a/superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts +++ b/superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts @@ -59,6 +59,7 @@ export enum FeatureFlag { Thumbnails = 'THUMBNAILS', UseAnalagousColors = 'USE_ANALAGOUS_COLORS', ForceSqlLabRunAsync = 'SQLLAB_FORCE_RUN_ASYNC', + SlackEnableAvatars = 'SLACK_ENABLE_AVATARS', } export type ScheduleQueriesProps = { diff --git a/superset-frontend/src/components/FacePile/index.tsx b/superset-frontend/src/components/FacePile/index.tsx index ae5a53d6870db..d67d9d8d36897 100644 --- a/superset-frontend/src/components/FacePile/index.tsx +++ b/superset-frontend/src/components/FacePile/index.tsx @@ -20,6 +20,7 @@ import { getCategoricalSchemeRegistry, styled, isFeatureEnabled, + FeatureFlag, SupersetTheme, } from '@superset-ui/core'; import { Tooltip } from 'src/components/Tooltip'; @@ -54,14 +55,13 @@ const StyledGroup = styled(Avatar.Group)` `; export default function FacePile({ users, maxCount = 4 }: FacePileProps) { - const enableAvatars = isFeatureEnabled('SLACK_ENABLE_AVATARS'); return ( {users.map(({ first_name, last_name, id }) => { const name = `${first_name} ${last_name}`; const uniqueKey = `${id}-${first_name}-${last_name}`; const color = getRandomColor(uniqueKey, colorList); - const avatarUrl = enableAvatars + const avatarUrl = isFeatureEnabled(FeatureFlag.SlackEnableAvatars) ? `/api/v1/user/${id}/avatar.png` : undefined; return ( From d63d0b5625036b72ee5fd344158a11b04a3bfd12 Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Mon, 16 Sep 2024 11:42:26 -0700 Subject: [PATCH 5/7] Add section+entry in FEATURE_FLAGS.md --- RESOURCES/FEATURE_FLAGS.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/RESOURCES/FEATURE_FLAGS.md b/RESOURCES/FEATURE_FLAGS.md index 57037d8797572..08fecdbcda1b3 100644 --- a/RESOURCES/FEATURE_FLAGS.md +++ b/RESOURCES/FEATURE_FLAGS.md @@ -93,3 +93,13 @@ These features flags currently default to True and **will be removed in a future - DASHBOARD_CROSS_FILTERS - ENABLE_JAVASCRIPT_CONTROLS - KV_STORE + +## Configuration Flags + +Currently some of our feature flags act as dynamic configurations that can changed +on the fly. This acts in contradiction with the typical ephemeral feature flag use case, +where the flag is used to mature a feature, and eventually deprecated once the feature is +solid. Eventually we'll likely refactor these under a more formal "dynamic configurations" managed +independently. This new framework will also allow for non-boolean configurations. + +- SLACK_ENABLE_AVATARS (see `superset/config.py` for more information) From 9420acf78ebc5c244b9daa6850a6fb18a56e7f6e Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Mon, 16 Sep 2024 13:05:45 -0700 Subject: [PATCH 6/7] docs --- RESOURCES/FEATURE_FLAGS.md | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/RESOURCES/FEATURE_FLAGS.md b/RESOURCES/FEATURE_FLAGS.md index 08fecdbcda1b3..d6fa8cccb5007 100644 --- a/RESOURCES/FEATURE_FLAGS.md +++ b/RESOURCES/FEATURE_FLAGS.md @@ -68,6 +68,13 @@ These features flags are **safe for production**. They have been tested and will - DISABLE_LEGACY_DATASOURCE_EDITOR ### Flags retained for runtime configuration + +Currently some of our feature flags act as dynamic configurations that can changed +on the fly. This acts in contradiction with the typical ephemeral feature flag use case, +where the flag is used to mature a feature, and eventually deprecated once the feature is +solid. Eventually we'll likely refactor these under a more formal "dynamic configurations" managed +independently. This new framework will also allow for non-boolean configurations. + - ALERTS_ATTACH_REPORTS - ALLOW_ADHOC_SUBQUERY - DASHBOARD_RBAC [(docs)](https://superset.apache.org/docs/using-superset/creating-your-first-dashboard#manage-access-to-dashboards) @@ -82,6 +89,7 @@ These features flags are **safe for production**. They have been tested and will - SQLLAB_BACKEND_PERSISTENCE - SQL_VALIDATORS_BY_ENGINE [(docs)](https://superset.apache.org/docs/configuration/sql-templating) - THUMBNAILS [(docs)](https://superset.apache.org/docs/configuration/cache) +- SLACK_ENABLE_AVATARS (see `superset/config.py` for more information) ## Deprecated Flags @@ -93,13 +101,3 @@ These features flags currently default to True and **will be removed in a future - DASHBOARD_CROSS_FILTERS - ENABLE_JAVASCRIPT_CONTROLS - KV_STORE - -## Configuration Flags - -Currently some of our feature flags act as dynamic configurations that can changed -on the fly. This acts in contradiction with the typical ephemeral feature flag use case, -where the flag is used to mature a feature, and eventually deprecated once the feature is -solid. Eventually we'll likely refactor these under a more formal "dynamic configurations" managed -independently. This new framework will also allow for non-boolean configurations. - -- SLACK_ENABLE_AVATARS (see `superset/config.py` for more information) From 87e2ac4b7cb4ae20ad342cd2dec8afa20e5414b6 Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Mon, 16 Sep 2024 13:38:32 -0700 Subject: [PATCH 7/7] sort --- RESOURCES/FEATURE_FLAGS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RESOURCES/FEATURE_FLAGS.md b/RESOURCES/FEATURE_FLAGS.md index d6fa8cccb5007..f985ad7254941 100644 --- a/RESOURCES/FEATURE_FLAGS.md +++ b/RESOURCES/FEATURE_FLAGS.md @@ -86,10 +86,10 @@ independently. This new framework will also allow for non-boolean configurations - ESCAPE_MARKDOWN_HTML - LISTVIEWS_DEFAULT_CARD_VIEW - SCHEDULED_QUERIES [(docs)](https://superset.apache.org/docs/configuration/alerts-reports) +- SLACK_ENABLE_AVATARS (see `superset/config.py` for more information) - SQLLAB_BACKEND_PERSISTENCE - SQL_VALIDATORS_BY_ENGINE [(docs)](https://superset.apache.org/docs/configuration/sql-templating) - THUMBNAILS [(docs)](https://superset.apache.org/docs/configuration/cache) -- SLACK_ENABLE_AVATARS (see `superset/config.py` for more information) ## Deprecated Flags