Create a new RefSpec alternative to EnvSpec #1291
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The SpecFile::EnvLayersFile flavor is likely not handled correctly. Signed-off-by: J Robert Ray <[email protected]>
The `EnvSpec` type has become overloaded and used is contexts where it can represent things that don't make sense. For example, `"-"` is a legal (empty) `EnvSpec` which can be used to create an empty environment, but it doesn't make sense to `spfs push` an empty environment. A `RefSpec` is a similar type but instead of representing items that can be used to create an environment, it represents items that can be copied between repositories. The `EnvSpec` scope has narrowed to restrict that any digests within it are treated as object digests (not payloads), because it is non-sensical to create an environment from a payload digest. The `RefSpec` allows digests to be payload digests, but does not support live layer files. It is also required to be non-empty, since it doesn't make sense to copy "nothing" between repositories. Spec files are still supported although the files themselves cannot be copied, however it should be possible to provide the list of refs to sync to `spfs push` via a spec file. # Design Notes There are still several places that accept an `EnvSpec` that is intended to be used to create an environment, but it is first used to sync items locally, so it needs to be converted to a `RefSpec` first (and the sync result converted back to an `EnvSpec`). These conversions are either lossy or fallible, so there are things likely broken in this PR that just aren't covered by tests yet. In this design, it makes sense for the conversion from `EnvSpec` to `RefSpec` to be lossy (instead of fallible) in the context of syncing content locally. These changes were created to build on the ideas in PR #1289 and further leverage `PartialDigestType` introduced there. Signed-off-by: J Robert Ray <[email protected]>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
EnvSpectype has become overloaded and used is contexts where itcan represent things that don't make sense. For example,
"-"is alegal (empty)
EnvSpecwhich can be used to create an emptyenvironment, but it doesn't make sense to
spfs pushan emptyenvironment.
A
RefSpecis a similar type but instead of representing items that canbe used to create an environment, it represents items that can be copied
between repositories.
The
EnvSpecscope has narrowed to restrict that any digests within itare treated as object digests (not payloads), because it is non-sensical
to create an environment from a payload digest.
The
RefSpecallows digests to be payload digests, but does not supportlive layer files. It is also required to be non-empty, since it doesn't
make sense to copy "nothing" between repositories. Spec files are still
supported although the files themselves cannot be copied, however it
should be possible to provide the list of refs to sync to
spfs pushvia a spec file.
Design Notes
There are still several places that accept an
EnvSpecthat is intendedto be used to create an environment, but it is first used to sync items
locally, so it needs to be converted to a
RefSpecfirst (and the syncresult converted back to an
EnvSpec). These conversions are eitherlossy or fallible, so there are things likely broken in this PR that
just aren't covered by tests yet.
In this design, it makes sense for the conversion from
EnvSpectoRefSpecto be lossy (instead of fallible) in the context of syncing contentlocally.
These changes were created to build on the ideas in PR #1289 and further
leverage
PartialDigestTypeintroduced there.