Change Layer interface from &self to &mut self #669
Conversation
schneems
force-pushed
the
schneems/spike-mut-layer
branch
from
8820a68
to
fe758cd
Compare
|
Here's a working PoC of a theoretical get/set based layer API heroku/buildpacks-ruby#203 |
schneems
force-pushed
the
schneems/spike-mut-layer
branch
2 times, most recently
from
6a42587
to
18aeaaa
Compare
The primary driver for wanting to mutate a layer is a desire for stateful logging. I hit this in the consuming state-machine interface, and previously, Josh hit it in a logging interface designed to track indentation level. Upside: - Developers are now allowed to do whatever they want within a layer - It's more technically correct to require a single reference for these functions. With the prior interface, it would be theoretically possible to execute the same Layer in parallel (though unlikely) - We can now change things between function calls. Downside: - Mutable self means there's no guarantee something didn't change between function calls.
schneems
force-pushed
the
schneems/spike-mut-layer
branch
from
18aeaaa
to
bbb6d9c
Compare
There was a problem hiding this comment.
Heya, thanks for re-suggesting this! Good to see that the change would just work without a more complex refactoring. Bottom line, I think we should make this change.
What I would like to see is a way to get the value passed to handle_layer back out, or allowing to pass a mutable reference to handle_layer. With the change in this PR, we can modify, but code outside of the layer will never be able to see what changed. When we think about the buildpack output, it would be cool to be able to get the current output state back out. This could be a separate PR or part of this one - I think the changes are closely related.
With the prior interface, it would be theoretically possible to execute the same Layer in parallel (though unlikely)
Since you needed to pass handle_layer an owned value, this would've not been possible. I also don't think that that would be a problem. The value implementing Layer is a description of a layer state and how to get there - it's not the layer itself. This is why the layer name is not part of the Layer trait. However, this is not really relevant to the change in this PR.
Mutable self means there's no guarantee something didn't change between function calls.
True, but all modifications would come from either the handle_layer implementation or the functions defined in Layer. This softens up the issue a lot for me. If you want to modify in your Layer implementation you have to work with that in the same layer - the issue is contained.
* Bump the libcnb group across 1 directory with 3 updates Bumps the libcnb group with 3 updates in the / directory: [libcnb](https://github.com/heroku/libcnb.rs), [libherokubuildpack](https://github.com/heroku/libcnb.rs) and [libcnb-test](https://github.com/heroku/libcnb.rs). Updates `libcnb` from 0.17.0 to 0.19.0 - [Release notes](https://github.com/heroku/libcnb.rs/releases) - [Changelog](https://github.com/heroku/libcnb.rs/blob/main/CHANGELOG.md) - [Commits](heroku/libcnb.rs@v0.17.0...v0.19.0) Updates `libherokubuildpack` from 0.17.0 to 0.21.0 - [Release notes](https://github.com/heroku/libcnb.rs/releases) - [Changelog](https://github.com/heroku/libcnb.rs/blob/main/CHANGELOG.md) - [Commits](heroku/libcnb.rs@v0.17.0...v0.21.0) Updates `libcnb-test` from 0.17.0 to 0.19.0 - [Release notes](https://github.com/heroku/libcnb.rs/releases) - [Changelog](https://github.com/heroku/libcnb.rs/blob/main/CHANGELOG.md) - [Commits](heroku/libcnb.rs@v0.17.0...v0.19.0) --- updated-dependencies: - dependency-name: libcnb dependency-type: direct:production update-type: version-update:semver-minor dependency-group: libcnb - dependency-name: libherokubuildpack dependency-type: direct:production update-type: version-update:semver-minor dependency-group: libcnb - dependency-name: libcnb-test dependency-type: direct:production update-type: version-update:semver-minor dependency-group: libcnb ... Signed-off-by: dependabot[bot] <support@github.com> * Change to mutable layers The layer trait interface was changed to mutable in heroku/libcnb.rs#669 * Switch from stacks to targets AKA "Stay on target" Buildpack API 0.10 removed the concept of stacks in favor of targets heroku/libcnb.rs#773. This commit works to upgrade applications in place by migrating metadata to support the new serialization format. This is supported by implementing TryMigrate from the `magic_migrate` crate https://docs.rs/magic_migrate/latest/magic_migrate/macro.try_migrate_link.html. https://www.youtube.com/watch?v=NnP5iDKwuwk * Add explicit Distro/Architecture cache messages * Use updated magic_migrate macros * Apply suggestions from code review Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com> * Document breaking changes * Align warning messages and error names * Update buildpack.toml - Add explanation for when stacks can be removed - Move "targets" closer to "stacks" as they're logically tied together. * Integration test for metadata migration * Support heroku-20 and heroku-22 (only) This buildpack does not support heroku-24 (yet) remove this stack from the TargetId struct. It also supports heroku-20 which was previously not present. * Fix changelog * Standardize println in integration tests * Consistent cache clear messages --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com>
The primary driver for wanting to mutate a layer is a desire for stateful logging. I hit this in the consuming state-machine interface, and previously, Josh hit it in a logging interface designed to track indentation level.
Upside:
Downside: