Should production have `config.assets.compile = false`

Discovered by @mikegreiling while troubleshooting https://gitlab.com/gitlab-org/gitlab-ce/issues/28743#note_24941249

created branch 29421-should-production-have-config-assets-compile-false

created branch 29421-production-config-assets-compile-false

mentioned in merge request !9917 (merged)

Here's a bit of background, just to document this issue a bit more.

In omnibus-gitlab!1264 (merged) we made some changes to allowed the omnibus package to ship with pre-compiled frontend assets so that we could remove NodeJS as a runtime dependency.

To cap this off, we added the EXECJS_RUNTIME=Disabled environment variable (see https://gitlab.com/gitlab-org/omnibus-gitlab/issues/1874#note_22311637). This was to allow gems which depend on the execjs gem to load without complaining. Even though there should be no circumstances in which the assets would need to be compiled on-the-fly in production, these gems would still check for a JS runtime when they got loaded.

However, since we still have config.assets.compile = true in production we open ourselves to the possibility that something might trigger Sprockets to think an asset is missing or outdated. When this happens, it will attempt to compile it, and without a javascript runtime it will fail and produce a 500 error.

This has apparently happened in a few corner cases, and it has led to some hard-to-diagnose errors: #28602 (closed), #28630 (moved).

Setting config.assets.compile = false in production prevents Sprockets from obfuscating these errors. If something is wrong with the assets, it would be far more helpful for it to complain about a missing asset rather than to attempt (and fail) to compute the asset on-the-fly.

---

Unrelated to this, there are also negative performance implications of using config.assets.compile = true in production.

See: http://stackoverflow.com/a/8827757/1447303

This feature is meant for development only, and enabling it produces a lot of overhead as every asset needs to be checked for changes every time it is served.

Right, I think it should be disabled in production, but I do wonder if there are other pages that quietly depend on this.

Interestingly enough, that line has been there since the very first commit of GitLab: 9ba12248

@stanhu I agree, it's possible some potential errors have been going on unnoticed due to this setting. I think if we merge this into master and hold off on picking into stable for a week or two, we'd at least be able to test this out on dev.gitlab.org and ensure that nothing major breaks because of it. Then we could Pick into Stable for 8.17 and 9.0.

changed milestone to %8.17

added 2fa label

removed 2fa label

removed milestone

mentioned in issue #28630 (moved)

closed via merge request !9917 (merged)