gabotorresruiz commented on code in PR #43805:
URL: https://github.com/apache/superset/pull/43805#discussion_r4066158975


##########
UPDATING.md:
##########
@@ -826,19 +826,34 @@ Note that a retried query returns partial data with no 
truncation indicator
 (e.g. a filter dropdown may list only a subset of values on tables above the
 row cap).
 
-### Dashboard "Export Data to Excel" requires a Celery worker and S3 bucket
+### Dashboard "Export Data to Excel" requires a Celery worker and a storage 
bucket

Review Comment:
   You are right, and this was the gap with the widest blast radius in the 
change. Fixed in 89e96fe8bbc43c84457ea7e5ae017a39d3a3eab6.
   
   I checked what the entry was actually missing: `EXCEL_EXPORT_S3_BUCKET`, 
`EXCEL_EXPORT_S3_KEY_PREFIX` and `EXCEL_EXPORT_S3_CLIENT_KWARGS` are the only 
config keys this change removes, and none of the three appeared anywhere in 
UPDATING.md. So an operator upgrading with a working export keeps a config that 
still looks valid, nothing reads it any more, and every export returns `501` 
with nothing pointing at the cause.
   
   The entry is now titled as a migration rather than as new configuration, and 
spells the mapping out:
   
   | Removed | Replacement |
   | --- | --- |
   | `EXCEL_EXPORT_S3_BUCKET` | `EXPORT_STORAGE["bucket"]` |
   | `EXCEL_EXPORT_S3_KEY_PREFIX` | `EXPORT_STORAGE["key_prefix"]` |
   | `EXCEL_EXPORT_S3_CLIENT_KWARGS` | `S3ExportStorage(client_kwargs=...)` |
   
   It also calls out the part an upgrade cannot infer, that `backend` has no 
default and the old config implied S3, and that `EXCEL_EXPORT_LINK_TTL_SECONDS` 
keeps its name but now bounds a Superset-issued link rather than a pre-signed 
S3 URL, so the AWS seven day ceiling no longer applies. The same note is on the 
docs page.
   
   If you would rather this not break upgraders at all, the alternative is a 
shim that reads the old keys into `EXPORT_STORAGE` with a deprecation warning. 
I did not add one because you asked for the migration to be spelled out, and a 
silent fallback would keep the S3-only assumption alive, but I am happy to add 
it if you prefer a soft landing.
   



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to