chore: move SLACK_ENABLE_AVATARS from config to feature flag #30274
Conversation
dosubot
bot
added
change:backend
docker-compose.yml
Outdated
| @@ -149,7 +149,14 @@ services: | |||
| disable: true | |||
|
|
|||
| superset-node: | |||
| image: node:20 | |||
| build: | |||
There was a problem hiding this comment.
bycatch - somehow superset-node wasn't using the proper target, which explains some issues with zstd we've seen before, which gets installed at the os-level here. There's also the "memory snag" script and the DEV_MODE logic to skip the actual webpack build since we run npm run dev on startup.
There was a problem hiding this comment.
I thought I had caught/fixed that in some other PR, but guessing it got lost in the shuffle.
There was a problem hiding this comment.
Please make sure it gets an entry in FEATURE_FLAGS.md as well :)
Pinging @eschutho for review since she's been handling most of the Slack integration scope.
Also, this might bring up the age-old debate of config vs. feature flags. I'm OK with Feature Flags for cases like this where platforms (Launch Darkly, Split, etc) can hotwire them as needed. Others, however, feel that Feature Flags should be temporary as a development pattern before being eventually deprecated/removed. Sounds like this one is effectively permanent.
| # 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 | ||
|
|
There was a problem hiding this comment.
Since this hasn't been released in an official version, I think this breaking change is ok.
There was a problem hiding this comment.
Right, and I added a note in UPDATING.md as a heads-up
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.
mistercrunch
force-pushed
the
ff_slack
branch
from
4b10be3
to
842e9ae
Compare
sadpandajoe
added
the
v4.1
(cherry picked from commit f315a4f)
Needed to make SLACK_ENABLE_AVATARS more dynamic @ Preset so it's easy for us to turn this feature on/off and allow us for a progressive rollout.