Vitor-Avila commented on code in PR #31798:
URL: https://github.com/apache/superset/pull/31798#discussion_r1913250154
##########
superset/dashboards/api.py:
##########
@@ -1141,6 +1130,7 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any)
-> WerkzeugResponse:
)
@expose("/<pk>/cache_dashboard_screenshot/", methods=("POST",))
+ @validate_feature_flags(["THUMBNAILS",
"ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS"])
Review Comment:
@geido if we remove the requirement to have `THUMBNAILS` enabled for this
endpoint, but then we keep it at the `digest` endpoint, then at the end the
process would still not work unless you have both enabled.
Also, currently when you trigger a "screenshot generation" it returns the
digest link, which returns a `404` status code until it's being processed. If
`THUMBNAILS` is disabled, it would return `404` forever and users wouldn't have
an easy way to spot the issue.
I think we could either:
* Remove the `THUMBNAILS` check for this endpoint, and then update the check
on the `digest` endpoint to validate for either `THUMBNAILS` OR
`ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS`. This way you can still download a
dashboard screenshot without having `THUMBNAILS` FF enabled; or
* Update the other endpoint to return a different status code if the
`THUMBNAILS` FF is disabled. Also update the frontend (if that's not the case
yet) to validate if both FFs are enabled to decide if frontend or backend
processing should be used.
One potential issue with the first approach is that environments with
`ENABLE_DASHBOARD_SCREENSHOT_ENDPOINTS` enabled but `THUMBNAILS` disabled would
still expose the `digest` endpoint, so it's possible that disabling
`THUMBNAILS` FF would no longer be sufficient to remove thumbnails from
displaying in the card views.
Let me know your thoughts!
--
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]
