-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
chore: add talisman env var to config #24774
chore: add talisman env var to config #24774
Conversation
/testenv up |
@eschutho Ephemeral environment spinning up at http://54.69.71.94:8080. Credentials are |
/testenv up |
Codecov Report
@@ Coverage Diff @@
## master #24774 +/- ##
=======================================
Coverage 68.99% 69.00%
=======================================
Files 1903 1906 +3
Lines 74073 74145 +72
Branches 8193 8208 +15
=======================================
+ Hits 51108 51164 +56
- Misses 20844 20858 +14
- Partials 2121 2123 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@eschutho Ephemeral environment spinning up at http://35.90.210.76:8080. Credentials are |
f083b3a
to
5530c7c
Compare
@@ -66,6 +66,8 @@ def get_env_variable(var_name: str, default: Optional[str] = None) -> str: | |||
REDIS_CELERY_DB = get_env_variable("REDIS_CELERY_DB", "0") | |||
REDIS_RESULTS_DB = get_env_variable("REDIS_RESULTS_DB", "1") | |||
|
|||
TALISMAN_ENABLED = get_env_variable("TALISMAN_ENABLED", "False") |
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 think this should be defined in superset/config.py
as well so in superset/views/base.py
you would do conf["TASLISMAN_ENABLED"]
rather than conf.get("TALISMAN_ENABLED", ...)
.
superset/views/base.py
Outdated
@@ -120,6 +120,7 @@ | |||
"ALERT_REPORTS_DEFAULT_RETENTION", | |||
"ALERT_REPORTS_DEFAULT_WORKING_TIMEOUT", | |||
"NATIVE_FILTER_DEFAULT_ROW_LIMIT", | |||
"TALISMAN_ENABLED", |
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.
Why do we need to expose this configuration variable to the frontend as there's no reference to this in the frontend?
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.
Sorry for the confusion. I just marked the PR as draft again. I'm just using that as a test to see if the config is being applied in the ephemeral environment or not.
/testenv up |
@eschutho Ephemeral environment spinning up at http://54.191.36.34:8080. Credentials are |
ffca5dd
to
bf2809c
Compare
/testenv up |
@eschutho Ephemeral environment spinning up at http://54.244.199.30:8080. Credentials are |
/testenv up |
@eschutho Ephemeral environment spinning up at http://52.27.244.206:8080. Credentials are |
67dd55c
to
09f5686
Compare
/testenv up |
@eschutho Ephemeral environment spinning up at http://54.218.124.88:8080. Credentials are |
@michael-s-molina it looks like it got this to work, if you want to review. |
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.
Nice! Thanks for working on this and for the added tests!
@justinpark once this PR is merged we'll be able to spin test environments again. |
Ephemeral environment shutdown and build artifacts deleted. |
(cherry picked from commit d23b20e)
SUMMARY
This allows us to pass the talisman config from the ephemeral docker image environment variable into the config so that we can turn off talisman for ephemerals.
The Talisman os env var for the ephemeral instance was added here: #24627
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION