Consolidate asset config

[davidsbailey]

Description

This PR consolidates the following configuration parameters/logic into a single variable CDO.optimize_rails_assets:

• config.assets.compile
• config.assets.digest
• whether to rake assets:precompile

This PR also eliminates some complexity from drone ui test config:

• eliminates redundant rake circle: recompile_assets step, because assets are now precompiled during rake build
• eliminates separate groupings of locals.yml params, which fixes UI tests that depend on CDO.override_pegasus being set during asset compilation

Having done all that, we also have to also

• enable CDO.optimize_webpack_assets for drone UI tests

because our servers do not work properly in the configuration where optimize_webpack_assets is false and optimize_rail_assets is true. This is because webpack relies on the assets it produces (in dashboard/public/blockly) being passed through the rails asset pipeline (to dashboard/public/assets) without rails adding its own digest, and our mechanism for doing this is to skip adding a rails digest only when a webpack hash is already present, as described here:

code-dot-org/dashboard/lib/tasks/asset_sync.rake

Line 47 in 9aa96b0

# Patch Sprockets to skip digesting already-digested webpack ('wp') assets.

I can't quite explain why the problem described in the previous paragraph was not causing all ui tests to fail in drone runs prior to this PR, but I do feel good about eliminating this poorly-understood configuration and making drone run more similarly to our test and production servers.

Alternative

Alternately, we could run drone ui tests without optimize_webpack_assets or optimize_rails_assets, to make drone runs more like local development. This scenario failed with unclear errors when I tried it earlier, but if we prefer to head in that direction I could investigate it as a next step.

Links

• see improve logic in webpack asset handler #31216 for similar past work, which consolidated CDO.optimize_webpack_assets
• further background: our js in the apps/ directory is passed first through webpack, and then through rails asset pipeline (a.k.a. sprockets). For documentation of this process, see: https://github.com/code-dot-org/code-dot-org/blob/9a14b8b40561ae01f6ab7aac4fed28f8b107595b/apps/docs/build.md#brief-production-build-overview

Testing story

No new tests because no new functionality has been added. I am relying on drone + DTT to catch any breakages.

Reviewer Checklist:

• Tests provide adequate coverage
• Code is well-commented
• New features are translatable or updates will not break translations
• Relevant documentation has been added or updated
• User impact is well-understood and desirable
• Pull Request is labeled appropriately
• Follow-up work items (including potential tech debt) are tracked and linked

[davidsbailey]

It looks like this line was intended to clear dashboard assets between CircleCI test runs, although it's hard to know whether it was effective. @uponthesun , any idea whether files in dashboard/public/assets are shared between drone test runs via any caching mechanism? If so, I could add a a step to remove all existing assets before rake build in ui_tests.sh.

[uponthesun]

Nothing is saved between runs currently 👍

[davidsbailey]

these removed lines had no effect because they were "conditionally" setting these to configuration parameters to their default values. This means that we've actually already been using digested / "optimized" assets in drone ui tests.

[uponthesun]

Looks like a great improvement to me, and I'm happy to review drone-related things for this, but it'd make sense if you wanted another reviewer who has more asset pipeline knowledge as well.

[uponthesun]

Update comment?

[uponthesun]

Nothing is saved between runs currently 👍

[uponthesun]

Also in this change, we set this to true in locals.yml for drone runs, right? So during a drone run this will evaluate to false here, but then will get overridden to true anyway? If so it'd be clearer to use !unit_test instead for the condition (and update the comment also). Unless there's some other case covered by ci_test...

[davidsbailey]

true for ui tests, but not unit tests. still, removing the double negative simplifies the logic a bit.

[davidsbailey]

Looks like a great improvement to me, and I'm happy to review drone-related things for this, but it'd make sense if you wanted another reviewer who has more asset pipeline knowledge as well.

@islemaster would you mind providing a sanity check?

[islemaster]

Repeating things back to make sure I understand them, after this change we have three valid configurations:

# (A) production-like
optimize_rails_assets: true
optimize_webpack_assets: true

# (B) local development default
optimize_rails_assets: false
optimize_webpack_assets: false

# (C) production-like JS but Rails local development mode
optimize_rails_assets: false
optimize_webpack_assets: true

And one unsupported configuration:

# (D) Broken because Rails expects webpack-provided digests but webpack isn't digesting.
optimize_rails_assets: true
optimize_webpack_assets: false

Questions I still have:

1. What is configuration (C) useful for?
2. What does using a prebuilt apps package look like in this world?
3. Should we collapse these two flags into an enum to eliminate the invalid configuration?

[davidsbailey]

Your understanding matches mine. To answer your questions:

1. configuration (C) is useful in development, for the specific purpose of debugging optimized JS, without having to jump through the extra hoop of precompiling dashboard assets and restarting dashboard server.

2. a prebuilt apps package is always built with optimize_webpack_assets, therefore any dashboard server using a prebuilt package must also look for optimized js via the webpack manifest. We enforce this here:

   code-dot-org/lib/cdo/asset_helper.rb

   Lines 35 to 37 in 34cc083

   	def webpack_asset_path(asset)
   	using_prebuilt_apps = !CDO.use_my_apps
   	use_manifest = CDO.optimize_webpack_assets || using_prebuilt_apps

   this effectively overrides optimize_webpack_assets when (and only when) using a prebuilt apps package in local development.

3. I find myself dragging my feet a little here. Two booleans seem simpler to reason about to me, and I'd argue that an enum would not substantially reduce chance of error because every enum has a potential invalid state when an invalid value is supplied, or when someone accidentally checks it as though it was a boolean. If we go this route, would each state in the enum map to a set of valid values for each config param? or would our code contain checks like optimize_webpack = [:optimize_webpack, :optimize_all].include? CDO.optimize_js? If we go the enum route, would that approach extend gracefully to include these related booleans?

   • use_my_apps (which itself seems like it could be combined as-is with use_my_apps), and

   • the logic for whether to upload an apps package:

     code-dot-org/lib/rake/package.rake

     Lines 6 to 7 in 7dc33f6

     	namespace :package do
     	BUILD_PACKAGE = %i[staging test adhoc].include?(rack_env) && !ENV['CI']

   I like the idea of eliminating invalid states in theory, but I'm having trouble seeing it eliminate any risk or complexity in this case.

[islemaster]

I like the enum approach not because it decreases risk or complexity, but because it forces us to name the composite configurations, documenting their meaning. I see what you're saying about the effect on the code wherever we use these configurations though.

I think an enum would reduce the complexity as soon as we pulled a third boolean into the mix. For instance, adding use_my_apps gives us eight possible configurations, but only five unique valid behaviors.

We might get just as much value out of early detection and messaging of invalid configurations though (as you've done here).

[islemaster]

In any case, this is definitely an improvement. 🎉

[davidsbailey]

Thanks for the thorough analysis, @islemaster . Let's keep enums on the table for any future cleanup, then.