Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Stacked PR] Schneems/layer output mut #762

Closed
wants to merge 4 commits into from

Conversation

schneems
Copy link
Contributor

  • Pulls in the change from my &mut layer PR (first 2 commits)
  • Replaces inline_output with a LayerOutput trait (next commit)
  • Adds logging to an example in the examples repos (next commit)

Need

I can get info into the layer now that it is mutable, but the result of context.handle_layer is a LayerData which does not retain a reference to the original layer. One option could be making LayerData take two generics instead of one, but this has large API consequences.

schneems and others added 4 commits January 29, 2024 14:34
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.
Status: I can get it into the layer, but cannot get it back out. Need to add a LayerData.layer property.
@schneems schneems requested a review from a team as a code owner January 29, 2024 21:44
@schneems
Copy link
Contributor Author

Closing this without merging. We've decided to move forward in a different way:

The section_logger renamed inline_output and in this PR changed to a LayerOutput trait is fundamentally a hack. We could continue with this hack by modifying LayerData in such a way that allows us to retrieve a layer once it's done executing.

When we tried this work, it impacts the internals, it impacts the consumers, and ultimately it seems like a lot of work for something we might not ultimately want.

We talked about:

  • Get rid of the hacks i.e. remove inline_output, and don't replace it with anything
  • Merge the scoped work in schneems/output
  • Continue that work by re-adding in older features that were removed to help make the PR easier to review
  • Once that work is done and merged:
    • Implement buildpack logging with this new code in one or more Java buildpacks. These buildpacks do not log inside the layer at all (which, I feel they should, but that's not the main concern.)
    • Work towards a layer interface that allows pushing/pulling data and bubbling up output messages so that we do not need any hacks. This builds off of the work in Exploded layer proof of concept buildpacks-ruby#203 (though not 1:1). This work has been known as a "nice to have" but instead of effectively wasting time working on hacks and making the internals of LayerData worse, we're reprioritizing that work. It was already important, now it's more urgent as well.

At the end we'll have a new output and layer concept that go hand in hand. We can update the Ruby buildpack using this style to ensure it's suitable for all buildpacks.

@schneems schneems closed this Jan 30, 2024
@edmorley edmorley removed the request for review from a team February 2, 2024 13:54
@edmorley edmorley deleted the schneems/layer-output-mut branch February 8, 2024 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant