You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sebastianliebscher (via GitHub)" <gi...@apache.org> on 2023/09/02 04:21:39 UTC
[GitHub] [superset] sebastianliebscher opened a new pull request, #25166: fix: set UPLOAD_FOLDER to superset/static/uploads/
sebastianliebscher opened a new pull request, #25166:
URL: https://github.com/apache/superset/pull/25166
### SUMMARY
This PR fixes an incorrect default value for config variable `UPLOAD_FOLDER`:
- current `superset/app/static/uploads/`
- new `superset/static/uploads/`
Additionally:
- `superset/static/uploads/` is now checked into git (analog as `superset/static/assets/`)
- `superset/static/assets/` was initially checked into git years before, but in another commit (also years before) then .gitignored without removal from git (I think unintentionally)
This checks both directories into git, thus being empty default directories. Any content will be ignored.
```.gitignore
superset/static/assets/*
!superset/static/assets/.gitkeep
superset/static/uploads/*
!superset/static/uploads/.gitkeep
```
The config variables `IMG_UPLOAD_FOLDER` and `IMG_UPLOAD_URL` were never used (found nothing using full text search).
### TESTING INSTRUCTIONS
- unit + integrations tests
- `docker build . -t test`
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: correct value for config variable `UPLOAD_FOLDER` [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #25166:
URL: https://github.com/apache/superset/pull/25166#issuecomment-2604990474
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/25166?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 83.46%. Comparing base [(`76d897e`)](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`4c23c22`)](https://app.codecov.io/gh/apache/superset/commit/4c23c228081588966bd019199406a2c1dddf9cb3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1338 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #25166 +/- ##
===========================================
+ Coverage 60.48% 83.46% +22.97%
===========================================
Files 1931 546 -1385
Lines 76236 39283 -36953
Branches 8568 0 -8568
===========================================
- Hits 46114 32786 -13328
+ Misses 28017 6497 -21520
+ Partials 2105 0 -2105
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/25166/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/25166/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.81% <100.00%> (-0.36%)` | :arrow_down: |
| [javascript](https://app.codecov.io/gh/apache/superset/pull/25166/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/25166/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `75.98% <100.00%> (?)` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/25166/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `76.05% <100.00%> (?)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/25166/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.30% <100.00%> (-0.50%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/25166/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.46% <100.00%> (+19.97%)` | :arrow_up: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/25166/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `75.52% <100.00%> (?)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/25166/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.08% <100.00%> (+3.46%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/25166?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sebastianliebscher commented on pull request #25166: fix: set UPLOAD_FOLDER to superset/static/uploads/
Posted by "sebastianliebscher (via GitHub)" <gi...@apache.org>.
sebastianliebscher commented on PR #25166:
URL: https://github.com/apache/superset/pull/25166#issuecomment-1703693148
This PR is read for review, but may require further discussion. I'm not sure, but that could quite possibly be a breaking change. Possibly one would have to move important files? Or a mention in the release notes is sufficient?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sebastianliebscher commented on a diff in pull request #25166: fix: set UPLOAD_FOLDER to superset/static/uploads/
Posted by "sebastianliebscher (via GitHub)" <gi...@apache.org>.
sebastianliebscher commented on code in PR #25166:
URL: https://github.com/apache/superset/pull/25166#discussion_r1313714604
##########
.gitignore:
##########
@@ -50,22 +50,23 @@ env
venv*
env_py3
envpy3
-env36
local_config.py
/superset_config.py
/superset_text.yml
superset.egg-info/
superset/bin/supersetc
tmp
rat-results.txt
-superset/app/
Review Comment:
This is the incorrect path that contained `static/uploads/`. `superset/app/` shouldn't be a valid directory anyway. For developers, this change could potentially show up in their git repo as untracked files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sebastianliebscher commented on a diff in pull request #25166: fix: set UPLOAD_FOLDER to superset/static/uploads/
Posted by "sebastianliebscher (via GitHub)" <gi...@apache.org>.
sebastianliebscher commented on code in PR #25166:
URL: https://github.com/apache/superset/pull/25166#discussion_r1313714604
##########
.gitignore:
##########
@@ -50,22 +50,23 @@ env
venv*
env_py3
envpy3
-env36
local_config.py
/superset_config.py
/superset_text.yml
superset.egg-info/
superset/bin/supersetc
tmp
rat-results.txt
-superset/app/
Review Comment:
This is the incorrect path that contained `static/uploads/`. `superset/app/` shouldn't be a valid directory anyway. For developers, this change could potentially pop up affected files in their git repo as untracked files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sebastianliebscher commented on a diff in pull request #25166: fix: set UPLOAD_FOLDER to superset/static/uploads/
Posted by "sebastianliebscher (via GitHub)" <gi...@apache.org>.
sebastianliebscher commented on code in PR #25166:
URL: https://github.com/apache/superset/pull/25166#discussion_r1313714723
##########
superset/config.py:
##########
@@ -680,17 +680,12 @@ class D3Format(TypedDict, total=False):
# Image and file configuration
# ---------------------------------------------------
# The file upload folder, when using models with files
-UPLOAD_FOLDER = BASE_DIR + "/app/static/uploads/"
+UPLOAD_FOLDER = BASE_DIR + "/static/uploads/"
UPLOAD_CHUNK_SIZE = 4096
-# The image upload folder, when using models with images
-IMG_UPLOAD_FOLDER = BASE_DIR + "/app/static/uploads/"
Review Comment:
I couldn't find any use of these variables.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: correct value for config variable `UPLOAD_FOLDER` [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #25166:
URL: https://github.com/apache/superset/pull/25166#issuecomment-2310840318
We're working toward 5.0, and I think that's the safe time to get this merged (finally). I've put it on the kanban board, just FYI :) Thanks for the contributions!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: correct value for config variable `UPLOAD_FOLDER` [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #25166:
URL: https://github.com/apache/superset/pull/25166
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sebastianliebscher commented on a diff in pull request #25166: fix: set UPLOAD_FOLDER to superset/static/uploads/
Posted by "sebastianliebscher (via GitHub)" <gi...@apache.org>.
sebastianliebscher commented on code in PR #25166:
URL: https://github.com/apache/superset/pull/25166#discussion_r1313714604
##########
.gitignore:
##########
@@ -50,22 +50,23 @@ env
venv*
env_py3
envpy3
-env36
local_config.py
/superset_config.py
/superset_text.yml
superset.egg-info/
superset/bin/supersetc
tmp
rat-results.txt
-superset/app/
Review Comment:
This is the incorrect path that contained `uploads/`. `superset/app/` shouldn't be a valid directory anyway. For developers, this change could potentially show up in their git repo as untracked files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: correct value for config variable `UPLOAD_FOLDER` [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #25166:
URL: https://github.com/apache/superset/pull/25166#issuecomment-2604876101
@rusackas I rebased the PR and added an entry to `UPDATING.md`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sebastianliebscher commented on a diff in pull request #25166: fix: set UPLOAD_FOLDER to superset/static/uploads/
Posted by "sebastianliebscher (via GitHub)" <gi...@apache.org>.
sebastianliebscher commented on code in PR #25166:
URL: https://github.com/apache/superset/pull/25166#discussion_r1313714723
##########
superset/config.py:
##########
@@ -680,17 +680,12 @@ class D3Format(TypedDict, total=False):
# Image and file configuration
# ---------------------------------------------------
# The file upload folder, when using models with files
-UPLOAD_FOLDER = BASE_DIR + "/app/static/uploads/"
+UPLOAD_FOLDER = BASE_DIR + "/static/uploads/"
UPLOAD_CHUNK_SIZE = 4096
-# The image upload folder, when using models with images
-IMG_UPLOAD_FOLDER = BASE_DIR + "/app/static/uploads/"
Review Comment:
I couldn't find any usage of these variables.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: correct value for config variable `UPLOAD_FOLDER` [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #25166: fix: correct value for config variable `UPLOAD_FOLDER`
URL: https://github.com/apache/superset/pull/25166
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] sebastianliebscher commented on a diff in pull request #25166: fix: set UPLOAD_FOLDER to superset/static/uploads/
Posted by "sebastianliebscher (via GitHub)" <gi...@apache.org>.
sebastianliebscher commented on code in PR #25166:
URL: https://github.com/apache/superset/pull/25166#discussion_r1313714046
##########
.gitignore:
##########
@@ -50,22 +50,23 @@ env
venv*
env_py3
envpy3
-env36
local_config.py
/superset_config.py
/superset_text.yml
superset.egg-info/
superset/bin/supersetc
tmp
rat-results.txt
-superset/app/
# Node.js, webpack artifacts, storybook
*.entry.js
*.js.map
node_modules
npm-debug.log*
-superset/static/assets
+superset/static/assets/*
+!superset/static/assets/.gitkeep
+superset/static/uploads/*
+!superset/static/uploads/.gitkeep
Review Comment:
This checks both directories into git, thus being empty default directories. Any content will be ignored.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: correct value for config variable `UPLOAD_FOLDER` [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #25166:
URL: https://github.com/apache/superset/pull/25166#issuecomment-2581340266
Closing and reopening to get CI to spin again. @sebastianliebscher can you please add an entry to UPDATING.md to help folks who are upgrading mitigate any risks here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: correct value for config variable `UPLOAD_FOLDER` [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #25166:
URL: https://github.com/apache/superset/pull/25166#issuecomment-2581338590
/korbit-review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org