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

Refactor low-level layer operations #786

Merged
merged 17 commits into from
Mar 11, 2024
Merged

Conversation

Malax
Copy link
Member

@Malax Malax commented Feb 15, 2024

This PR refactors the internal functions in layer/handling.rs in preparation of a new Layer API PR. The only public (and strictly speaking breaking) change are the new error enums for writing SBOM and exec.d.

The initial PR that also exposed these low-level building blocked proved to be more controversial than I expected so this PR changed to only refactor the internal bits.

Old PR description

This PR is the first phase for a new high-level Layer API. It exposes the low-level building blocks needed to work with CNB layers. It is not the new layer API.

Why do we need this when we're aiming for a new high-level API?

First of all, that new high-level API needs access to these operations. They were originally written as helpers for the Layer trait API and and needed some adjustments to make them usable in other contexts. I mainly improved error handing and naming. This work needed to be done in any case and this PR front loads these changes in a separate PR for easier review. :)

Secondly, regardless of how great the new high-level API will be, there will always be fringe use-cases where low-level building blocks are needed. Right now, users of libcnb.rs are out of luck if they ever hit these uses-cases. After this PR, we have an escape hatch when a higher-level abstraction (either the current or new one) doesn't work.

Notes for reviewers

Start with the BuildContext changes in libcnb/src/build.rs. This is the newly exposed API for users. Everything else is just supporting changes.

I've broken up the steps I took in separate commits. It might be helpful to refer to those to see how things changed in atomic steps from the original.

@Malax Malax added enhancement New feature or request libcnb labels Feb 15, 2024
@Malax Malax force-pushed the malax/expose-low-level-layer-ops branch 3 times, most recently from 535fa5b to c4a3f5c Compare February 16, 2024 11:46
@Malax Malax force-pushed the malax/expose-low-level-layer-ops branch 2 times, most recently from 0540ca5 to 2a6bc55 Compare February 26, 2024 13:23
@Malax Malax marked this pull request as ready for review February 26, 2024 13:39
@Malax Malax requested a review from a team as a code owner February 26, 2024 13:39
Copy link
Contributor

@schneems schneems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting:

  • There's no way to write an environment variable that's exposed right now. I would like one to be added.
  • Sboms and execd can be written but not read (so not updated). I would like a way to read these values.

Proposing

I left some comments about possible invalid state if launch metadata isn't written. I know we've said that there will be a higher level interface for v2 but I'm warry about needing a function to expose each of the pieces of functionality. These associated functions, while not recommended, are on the BuildContext so users will certainly find them.

I think it's possible and preferable to reduce all these functions to one write and one read function. An update can be composed of a read followed by a modification and a read. Ideally the type that is returned by the read can be easily converted (with minimal extra data such as LayerTypes) into the type needed to write.

Such an "atomic" set of read/write operations would remove the need for checks and error types like 2a6bc55.

An alternative could be requiring LayerTypes input on every function call. Though I like that less, as there's an open question of what should happen in case the programmer issues two writes with different values.

libcnb/src/build.rs Outdated Show resolved Hide resolved
libcnb/src/layer/handling.rs Show resolved Hide resolved
libcnb/src/build.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
libcnb/src/build.rs Outdated Show resolved Hide resolved
@Malax Malax requested a review from schneems February 28, 2024 11:17
@Malax
Copy link
Member Author

Malax commented Feb 28, 2024

There's no way to write an environment variable that's exposed right now. I would like one to be added.

https://docs.rs/libcnb/latest/libcnb/layer_env/struct.LayerEnv.html#method.write_to_layer_dir

Sboms and execd can be written but not read (so not updated). I would like a way to read these values.

See comment here: #786 (comment)

Copy link
Contributor

@schneems schneems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've commented on each of your comments. Two of them can be closed. For the third:

I think we should error when LayerType-s are not configured for functions: replace_layer_exec_d_programs, replace_layer_sboms and EnvLayer::write_to_layer_dir

CHANGELOG.md Outdated Show resolved Hide resolved
libcnb/src/build.rs Outdated Show resolved Hide resolved
libcnb/src/build.rs Outdated Show resolved Hide resolved
@Malax Malax force-pushed the malax/expose-low-level-layer-ops branch from cc24fed to 750b26d Compare March 1, 2024 09:53
@Malax Malax changed the title Expose low-level layer operations Refactor low-level layer operations Mar 1, 2024
@Malax
Copy link
Member Author

Malax commented Mar 1, 2024

@schneems, @edmorley since the exposure of the low-level building blocks seems much more controversial than I expected, those changes have been removed from this PR. It's now only refactoring the internal functions and slightly changes WriteLayerError.

I still think we want to do this eventually (and some of it is needed for the new Layer API), but I want the internal changes merged ASAP to be able to open the actual new Layer API PR.

I will re-request review from you both. :)

@Malax Malax requested review from schneems and edmorley March 1, 2024 10:33
libcnb/src/layer/handling.rs Show resolved Hide resolved
Copy link
Contributor

@schneems schneems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see two changes:

  • Rename "overwrite" to "replace"
  • Extract functionality from write_layer to their own methods.

And an associated change to the error enum as documented in the CHANGELOG.

None of these changes are controversial. I'm happy to move forward. I'm okay moving the location of the enum. I would like docs for completeness sake and left a suggestion, but I also won't block on that either.

libcnb/src/layer/handling.rs Show resolved Hide resolved
libcnb/src/layer/handling.rs Outdated Show resolved Hide resolved
@Malax Malax enabled auto-merge (squash) March 11, 2024 11:07
@Malax Malax merged commit 6e89c73 into main Mar 11, 2024
4 checks passed
@Malax Malax deleted the malax/expose-low-level-layer-ops branch March 11, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants