-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
fix: Read MAPBOX_API_KEY from environment #16926
Conversation
@junlincc Hey, the workflow tests haven't started on their own in a while. Would it be possible to manually start these tests? |
@Lawful2002 I closed/reopened the PR, now CI checks seem to have started correctly, sorry for the delay. Will take a look once tests complete. |
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.
LGTM once pre-commit is passing!
Hey @Lawful2002 , there's only black formatting to fix and this PR can be merged as it's approved. It would be super useful to have it :) |
@@ -44,7 +44,7 @@ def get_env_variable(var_name: str, default: Optional[str] = None) -> str: | |||
) | |||
raise EnvironmentError(error_msg) | |||
|
|||
|
|||
MAPBOX_API_KEY = get_env_variable("MAPBOX_API_KEY") |
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.
Is this duplication of https://github.com/apache/superset/blob/master/superset/config.py#L695 required?
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'm not really sure if this duplication is required or not, if I'm being honest. However as the API key is already getting fetched in https://github.com/apache/superset/blob/master/superset/config.py#L695, this duplication may not be required. That said though, the linked issue (#16781) asked MAPBOX_API_KEY = get_env_variable("MAPBOX_API_KEY")
to be added to docker/pythonpath_dev/superset_config.py
. Also the documentation at https://github.com/apache/superset/blob/a330b664c13e34a4c7ca954f9a3a527a43a5535f/docs/src/pages/docs/frequently-asked-questions-page.mdx#why-is-the-map-not-visible-in-the-geospatial-visualization wants that key in that particular file/location.
I've fixed the formatting. Sorry for the delay |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue |
Not stale. |
Codecov Report
@@ Coverage Diff @@
## master #16926 +/- ##
===========================================
+ Coverage 66.47% 77.24% +10.76%
===========================================
Files 1727 973 -754
Lines 64724 50523 -14201
Branches 6822 6184 -638
===========================================
- Hits 43024 39025 -3999
+ Misses 19969 11292 -8677
+ Partials 1731 206 -1525
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@Lawful2002 there are some conflicts at the moment |
Wow, I'm not sure how this PR fell between the couch cushions for so long, but I'd love to merge this and close the related issue! I'm going to close the PR and re-open it to kick-start the CI process. Then feel free to nag me or others here or on Slack to get this thing merged! |
SUMMARY
This PR fixes: #16781
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
MAPBOX_API_KEY = get_env_variable("MAPBOX_API_KEY")
insuperset_config.py
MAPBOX_API_KEY=''
indocker/.env
anddocker/.env-non-dev
ADDITIONAL INFORMATION