Add Eq, PartialEq, Serialize, Deserialize to Target #823
Conversation
Signed-off-by: Josh W Lewis <josh.lewis@salesforce.com>
78d788f to
a15a19b
Compare
|
If we want to implement What are your thoughts about the additional attributes @joshwlewis? |
|
I chose not to use |
|
Fwiw I'm fine with making |
|
Short: I'm leaning towards not wanting Serialize and Deserialize implemented. I think Clone, Debug, Eq and PartialEq make sense as mechanical properties. My worry with allowing these to be serialized directly is that maintainers must be aware that changes to that struct can cause cache misses and users would get no compile errors or warnings and would have to check the changelog. We even recently changed the format of Target #821. Long:
I've also experimented a bit with auto-generating the "changed" warning messages based on the serialized key https://github.com/heroku/buildpacks-ruby/pull/203/files#diff-1cedf1bfbafd5ca2b85e8261de347a2318ab3ea87abe49eeff117fb3f380faa9R150-R154. I think that approach works best when the desired key/value is stored at the top level instead of nested within a struct. Another benefit to storing the data needed directly at the top level is migrating between metadata versions is fairly easy https://github.com/schneems/magic_migrate. However, if you need to start supporting multiple sub-migrations of associated structs, the picture gets much murkier. Those are all from the buildpack maintainer angle. From the libcnb maintainer angle, I don't feel like |
Seems fine. Will do!
I'm not following what's weird about both implementations. Care to elaborate?
This is a strong argument. There's a fair chance the data format changes in the future, which would potentially invalidate otherwise valid caches. In the go use case, I don't care a whole lot though, since it's all intermediate compilation artifacts which are invalidated frequently anyway (go patch version changes, n number of uses).
I see the value of being explicit here, but does every layer in every buildpack need to reimplement this logic?
The go buildpack doesn't explain why the build cache expired. It's supposed to be optional and ephemeral, so not a big deal. In short, I think this makes sense for Go, and possibly other buildpacks that need wholesale invalidation on a target change. Do we want to force buildpack authors to write this themselves? My opinion is that this is a pretty low-cost convenience in some scenarios, and totally optional - buildpack authors can still choose their own cache expiration. |
It looks like we really only do that for |
|
Given the resistance I'm seeing, I'll close this. It's not a big deal to handle target invalidation manually for one buildpack. |
Brainstorming another path forward:
I picked those three values because they're what Ruby and Python both needed.
|
Buildpacks (like heroku/buildpacks-go) may wish to store
Targetin layer metadata, so that caches may be invalidated whenTargetos,arch,distro, etc. changes. To store and retrieveTargetdata in toml, we needDeserializeandSerialize. To detect changes between two targets,PartialEqandEqare also implemented.