Avoid calling versioned_static from styleguide on application startup #11647
Conversation
Manage this branch in SquashTest this branch here: https://gasmanfixversionedstatic-on-st-08a4s.squash.io |
gasman
force-pushed
the
fix/versionedstatic-on-startup
branch
from
21d3353
to
148e14a
Compare
|
See https://github.com/wagtail/wagtail/actions/runs/7901444476 for an instance of this failure being triggered. |
| if os.environ.get("WAGTAIL_FAIL_ON_VERSIONED_STATIC", "0") == "1": | ||
|
|
||
| base_url = static(path) | ||
| def versioned_static(path): | ||
| raise Exception( |
There was a problem hiding this comment.
I'm not convinced this is the best way to test this... it seems too specific and doesn't really simulate the issue, which is doing collectstatic with DEBUG=False. If we don't have a similar setup in our CI, we potentially could run into similar issues that's not caused by versioned_static (e.g. the reason why we added collectstatic in our CI like #9002).
I've found a different approach that works by setting DEBUG = False via an environment variable, will make a separate PR now.
There was a problem hiding this comment.
Also, just to add: I agree it is nice to show a more helpful message like this when developers put versioned_static at startup, but the problem is they need to explicitly set WAGTAIL_FAIL_ON_VERSIONED_STATIC to 1, and neither this setting nor versioned_static are documented, so they have to know that this is an issue to detect the issue (if that makes sense). If it's documented (or defaults to 1), I wouldn't mind us having these :D
There was a problem hiding this comment.
This isn't really intended to be a test in the sense of replicating something that would happen on a real-world site - it's more of a quality check on the Wagtail codebase, like test_migrations, to ensure that CI fails loudly if this has been overlooked. It doesn't need to be documented, as it's not intended for developers to run it outside of the CI environment.
Defaulting to 1 wouldn't work, because that would break legitimate uses of versioned_static during runtime too. I don't think there's any good way to distinguish between valid and invalid uses of versioned_static in code, so the idea is that the CI runner enables the WAGTAIL_FAIL_ON_VERSIONED_STATIC flag for a command such as manage.py check or manage.py collectstatic that isn't expected to render any views - if this fails, we know it's an invalid use of versioned_static.
There was a problem hiding this comment.
Yeah, defaulting it to 1 would likely require some nasty hacks so that the function knows whether it's run at startup or not. OK then, in that case I'm happy to add this in. I still think it's worth having collectstatic run with DEBUG = False though (no need to backport that one), so I'll rebase my other PR after this is merged if that's OK.
|
Thanks all, my apologies for missing this when doing #11164 Glad we have an automated way to test for this in the future. |
Fixes #11643. Change
class Mediainto amediaproperty to prevent it from being called on application startup, and add a check to our CI to fail loudly if this happens in future.