Improve UX for cached layers that have forgotten to implement existing_layer_strategy()

[edmorley]

Something that came up in our team huddle today was that if a Layer has a types() that includes cache: true, then if the Layer doesn't implement existing_layer_strategy() the layer will silently not be cached, which can cause confusion.

To improve this, we could:

• Add additional more specific traits for common scenarios (eg CachedLayer), which can enforce that existing_layer_strategy() is implemented
• Add a runtime check to the default Layer::existing_layer_strategy implementation to fail if types() returns cache: true

cc @jonnymacs

[Malax]

FWIW, I got confused by this recently as well and had to dive quite deep to understand why my layer wasn't doing what I expected it to do (caching). I think we should talk again about the Layer::existing_layer_strategy default implementation, changing it to ExistingLayerStrategy::Keep would've prevented both my and @jonnymacs confusion.

This does not mean we shouldn't have a CachedLayer or similar.

[edmorley]

The problem with a default of ExistingLayerStrategy::Keep is that the default implementation won't be able to log a "Keeping X because Y" (or even just "Reusing cached layer"), since we have zero built-in logging ability in libcnb.

As such, it feels like "why is my thing not changing when I've changed my code" is going to be equally as hard to debug, and will replace the current confusion.

[Malax]

is going to be equally as hard to debug

For me, the difference here is that you explicitly write cache: true and then it does not cache with the current default implementation. For the 99% use case (the buildpack that wrote the initial layer is also the one handling the case it was restored from cache), the "why is my thing not changing when I've changed my code" is probably less confusing since it would only occur when you explicitly write cache: true in your Layer::types implementation.

[nokome]

FWIW I support addressing the confusion around existing_layer_strategy. I burnt several hours trying to figure out why my builds were not caching before stumbling across existing_layer_strategy in one of the examples. Either of @edmorley 's suggestion sound fine.

[edmorley]

the "why is my thing not changing when I've changed my code" is probably less confusing since it would only occur when you explicitly write cache: true in your Layer::types implementation.

To be clear, I mean a scenario where a cache:true buildpack implements create() and update(), but forgets to implement existing_layer_strategy() and then update() is never run. Now I agree it might be slightly easier to debug than a layer not being cached (eg the build log output won't have any output from the update() steps), however it would still be confusing/not great for UX.

I also can't think of a single case where it would be correct for a cache: true buildpack to rely on any default existing_layer_strategy() behaviour (no matter what default value we pick). i.e.: Pretty much any cached layer should invalidate on some condition (eg stack change, runtime change, pruning cache after N days/months etc), therefore should not be returning a static ExistingLayerStrategy value, but adjusting the value based on some conditional.

Therefore, having a default ExistingLayerStrategy return value for existing_layer_strategy() doesn't save boilerplate for any valid scenarios, and only adds to confusion.

As such, I think the default existing_layer_strategy() implementation should panic at runtime for cache: true layers. We can then further improve UX by adding more specific traits such as CachedLayer etc, that don't have a default implementation of existing_layer_strategy() at all.

We should probably also decide how far we want to support the "this buildpack didn't write this layer" / "a layer has changed from cache: true to cache: false" type scenarios, since if we decided to not support those (eg by just cleaning up old cached layers in those cases), perhaps we could simplify UX even more, and just drop the cache: true thing entirely, and use only traits to differentiate between cached and uncached layers.