manifest: introduce internal/manifest with private types and freeze public manifest.List

[flouthoc]

Flips dependency of internal and public manifest.List API and introduces private manifest.List API which will be extended in future for new features.

[flouthoc]

Upon discussion with @mtrmac I think this is something which he was suggesting. But @mtrmac could you PTAL and confirm if this is on the right track. Thanks

[mtrmac]

Could you prototype this a bit further, please?

In a new commit, add a do-nothing method to internal/manifest.List, and call it from copy.Image. Build the necessary infrastructure to do that; ensure that the new method can not be called via c/image/manifest.ListFromBlob; and then remove that do-nothing method.

In particular, I expect that will require at least creating internal/manifest.OCI1Index and internal/manifest.Schema2List; and that, in turn, might require moving quite a bit more types/constants/variables from manifest to internal/manifest.

The goal of this is to do all of the refactoring in this PR, so that the Zstd PR can just add a method without any more refactoring.

---

I guess the NonImageArtifactError type might need to be moved into a separate subpackage. Or, alternatively, move almost all of the implementation of c/image/manifest to internal/manifest, so that internal/manifest has never a reason to call the public one.

[mtrmac]

ensure that the new method can not be called via c/image/manifest.ListFromBlob

On second thought — callers that explicitly cast are not interesting; those deserve any breakage they get. But the new method should not be directly visible in the API (i.e. it should not be a public method on a publicly visible struct).

[flouthoc]

I guess the NonImageArtifactError type might need to be moved into a separate subpackage. Or, alternatively, move almost all of the implementation of c/image/manifest to internal/manifest, so that internal/manifest` has never a reason to call the public one.

@mtrmac Do you have any priority on these two suggested solution ? Like which one do you think could be better so I can do that part. IMO maybe moving all of the manifest implementation to internal/manifest is easier but it creates redundancy in codebase which we will have to slowly remove/reduce.

OTOH I guess the NonImageArtifactError type might need to be moved into a separate subpackage sounds less disruptive.

[mtrmac]

but it creates redundancy in codebase which we will have to slowly remove/reduce.

I was thinking that the public manifest package would not contain a duplicate implementation, just forwarding stubs, similarly to c/image/image.

---

My hunch is to try moving the NonImageArtifactError code to a separate subpackage; at a first glance it seems quite possible to move only the multi-arch code (implementations of manifest.List) without substantially affecting the single-instance code (s1/s2/OCI); that way, we wouldn’t need to add all the forwarding stubs now. It’s quite possible I have overlooked something and moving all of the single-instance code would be necessary as well.

[flouthoc]

Wrapping of structs is breaking the calling pattern for public API, I wonder if redundancy of structs is the only option ? https://go.dev/play/p/1Q_4X9hNB5j

Edit: Please ignore this comment , i think i got some idea.

[flouthoc]

In a new commit, add a do-nothing method to internal/manifest.List, and call it from copy.Image.

@mtrmac PTAL I think this is what you meant, I have also added a DoNothing() to private implementation in a new commit for proof-of-concept. If this looks good then I will drop the DoNothing commit in a newer push once we have agreement.

[mtrmac]

(This is not a review nor a request for any change, just a note about Go language features at this point.)

Wrapping of structs is breaking the calling pattern for public API, I wonder if redundancy of structs is the only option ?

type OCI1 = internalPackage.OCI1Public.

Compare e.g. pkg/compression/types.Algorithm.

[mtrmac]

Thinking back to the goals of this infrastructure:

• Primary: we must not change the public manifest.List interface, or other interfaces visible in manifest. This PR works fine for that.
• Not 100% necessary but desirable: minimize duplication between the public and internal manifest package, in particular to decrease the cost of maintenance.
• Not 100% necessary bu very desirable: Don’t expose the new APIs we need, at least while they are being developed for the full Zstd variants feature and still being shaped, as public API in manifest that we need to keep stable.

This PR works fine for the first point; but looking at the second two

• the existing public ChooseInstance methods, and the new methods with more options, should share code if at all possible (with the existing methods probably just calling to the new more capable implementation)
• that shared code should not live in the public manifest package

That’s a bit awkward to do with the current internal → public dependency direction. It would require moving the actual implementation logic into a third $implementation subpackage, so that the public package does not have new functions, but so that the actual implementation can still be called from manifest. And to move the actual implementation to $implementation, we would have to also move the data type to $implementation. (That would not be strictly necessary for OCI1, with a third-party package that contains the actual data definitions, but it would be necessary for schema2list).

The net effect of that would be internal/manifest (basically trivial wrappers) → manifest (trivial wrappers) → $implementation…

… so I think it would actually be much simpler to reverse the dependency:

• Make manifest depend on internal/manifest. No $implementation package is necessary.`
• Move all of the actual implementation from manifest to internal/manifest (manifest.OCI1Index = internal/manifest.OCI1IndexPublic)
• Let internal/manifest provide internal/manifest.OCI1 that contains an internal/manifest.OCI1IndexPublic member
• Existing public OCI1IndexPublic methods can stay public methods.
• Other shared implementations would end up as separate public functions. In particular,
  • A new internal/manifest.OCI1ChooseInstance(internal/manifest.OCI1Public function would be created contain the actual implementation
  • manifest/OCI1.ChooseInstance = internal/manifest.OCI1Public.ChooseInstance just calls OCI1ChooseInstance with the right parameters
  • A future new internal/manifest/OCI1.ChooseInstance[TheNewOne] is a trivial forwarder to internal/manifest.OCI1ChooseInstance.

Does that make sense? I think this would work and do everything necessary, but I didn’t actually do the work so I’m not 100% sure there isn‘t a snag.

[mtrmac]

• A new internal/manifest.OCI1ChooseInstance(internal/manifest.OCI1Public function would be created contain the actual implementation

Actually that can just be a package-private OCI1Public.chooseInstance method.

[flouthoc]

@mtrmac PTAL I agree moving everything to internal is much better. Also I have kept naming scheme as OCI1Index and OCI1IndexInternal for consistency with other packages and it automatically explains that the one with suffix Internal is the private type while other is public.

Also added a dummy commit to show how private API can be extended.

[mtrmac]

At this point let’s just discuss it, please don’t immediately make changes:

---

Is there a dependency that forces us to move the non-multi arch implementations as well?

---

Also I have kept naming scheme as OCI1Index and OCI1IndexInternal for consistency with other packages

We don’t really have that precedent AFAICS.

and it automatically explains that the one with suffix Internal is the private type while other is public.

The way I think about this is, most of the c/image code is going to use the internal API so saying “internal” all over the place is not particularly useful. Pointing out public parts of an otherwise-nonpublic API might be a thing to pay attention to.

[mtrmac]

Toying with https://github.com/mtrmac/image/tree/list-experiment (which is by no means complete), it does seem that we don’t actually need to move the non-multi-arch types. The only real dependency is a FromBlob in list_test.go, which can be avoided by keeping the original test in manifest, and dropping that one test part from a copy in internal/manifest.

So, yes, I’d prefer minimizing the churn and dealing with only the two multi-arch objects.

[flouthoc]

@mtrmac I was playing with your branch it correctly moves the two needed types and leaving everything else intact, what do else is left which you think is missing from that and needs to be done.

[mtrmac]

It seems to me that the structure works overall … so just a final cleanup:

• Explicitly note, everywhere, which symbols in internal/manifest are publicly visible
• Add individual documentation to the new symbols, and update documentation of the old symbols
• Keep the existing tests in c/image/manifest

[flouthoc]

@mtrmac PTAL again, I'll drop DoNothing( commit once PR is approved.

[mtrmac]

• Please add the “This is publicly available” comments to all such top-level items. Both data types like Schema2PlatformSpec and functions like MatchesDigest
• I’m afraid there was recently some churn on the main branch to benefit from Go 1.18, the recent rebase of this branch is reverting some of those. Please update. (FWIW that can be compared using e.g things like git diff {upstream/main:,other-branch:internal/}manifest/oci_index_test.go ).
• The second commit can be dropped now; we have proven the architecture out already, so let’s avoid another review iteration.

[mtrmac]

• I’m afraid there was recently some churn on the main branch to benefit from Go 1.18, the recent rebase of this branch is reverting some of those. Please update.

Similarly #1824 has a small intersection with this PR. I’m sorry about that hassle, it was necessary to allow building with Go 1.20 and enabling other work.

[mtrmac]

A few more comment updates, please.

LGTM aftewards, feel free to merge after tests pass.

[flouthoc]

@vrothberg @giuseppe @mtrmac PTAL

[vrothberg]

LGTM

[mtrmac]

@flouthoc Thanks!