API stability review

[mtrmac]

WIP list of packages/interfaces to be kept / made private / changed.

• S = we can commit to keeping this Stable
• I = this should become Internal
• U = Unstable, do we somehow declare it as such or change it to be able to keep it stable?
• D = Marked as Deprecated. TO DO: Find these, and decide whether to keep them.
• ? = unreviewed, undecided, or may need changes before committing to it

X	Element	Comment
S	c/i	N/A, docs only
S	c/i/copy	This must be stable, not sure about the current API though
?	c/i/directory
I	c/i/directory/explicitfilepath
?	c/i/docker
?	c/i/docker/archive
?	c/i/docker/daemon
I	c/i/docker/policyconfiguration
S	c/i/docker/reference	This must have stable semantics, has a long enough history, but do we need all of it
?	c/i/docker/tarfile	Should be internal, but libpod needs a part
?	c/i/image	Careful!
?	c/i/manifest	Careful!
S	c/i/oci	N/A
?	c/i/oci/archive
?	c/i/oci/layout
?	c/i/openshift	Deprecated already
?	c/i/pkg/blobinfocache
?	c/i/pkg/blobinfocache/boltdb
?	c/i/pkg/blobinfocache/memory
?	c/i/pkg/blobinfocache/none
?	c/i/pkg/compression	Would be nice to make this internal, just export opaque types
?	c/i/pkg/compression/types
?	c/i/pkg/docker/config
S	c/i/pkg/strslice
?	c/i/pkg/sysregistriesv2
?	c/i/pkg/tlsclientconfig
?	c/i/signature
?	c/i/storage
?	c/i/tarball
?	c/i/transports
?	c/i/transports/alltransports	Need this to be stable
?	c/i/types	Must provide something… but often opaque types would be better than full contents.
?	c/i/version	Need this to be stable.

---

Transport subpackage concerns:

For all the per-transport subpackages, we probably need to provide the Transport variable (but does that need to publicly support anything besides Name()?) Or maybe make public transport-specific reference (source? destination?) types so that callers that must distinguish transports after-the-fact can do so using a type check instead of name comparison, and discourage Name() comparison.

ParseReference/NewReference should be public in principle, but I’m not sure about committing to the all-in-one types.SystemContext. Maybe

• Split that into per-transport option types
• (Should the options be passed when creating an ImageReference or only ImageSource/ImageDestination? Moving that into ImageReference would allow making copy.Image SystemContext-independent.)
• Re-create types.SystemContext on top of the rest of c/image, like transports/alltransports, as a “create-reference-from-SystemContext-like-unstructured-options“ wrapper/helper.

This uncertainty about SystemContext of course impacts the ability to commit to NewReference/StableReference. OTOH, I’d much rather have stable per-transport NewReference(systemContext, transport-specific-options) than only to commit to string-syntax alltransports.ParseImageName as the only stable interface.

(Cc: @vrothberg )

[vrothberg]

Thanks a ton for putting this together, @mtrmac! For now, just a quick comment as I am still catching up with emails and GitHub.

Unstable, do we somehow declare it as such or change it to be able to keep it stable?

Although I initially liked the idea, I have changed my take on that. Marking certain parts as unstable would certainly make our lives much easier but it would turn into rather big troubles for downstream packagers. Debian, in particular, is putting all go packages into separate deb-source packages and they rely on semantic versioning. Assuming we would mark certain parts as unstable and would introduce breaking changes but not don't bump the major version, the Debian packages might not compile anymore.

[mtrmac]

Well, we can try… but it’s not quite clear that we can get that done.

Consider the most common cases of API breaks, where ImageDestination changes Method(OriginalParams) to Method(OriginalParams, ExtraParam). Sure, we can formalistically not break the API with

//// types/types.go
type ImageDestination interface {
    Method(OriginalParams) ReturnType
}
type ImageDestinationExt42 interface {
    ImageDestination
    MethodWithExtra(OriginalParams, ExtraParam) ReturnType
}

//// copy.go
func destMethod(dest ImageDestination, orig OriginalParams, extra ExtraParam) ReturnType {
    if dest, ok = dest.(ImageDestinationExt42) {
        return dest.MethodWithExtra(orig, extra)
    }
    return dest.Method(orig)
}
// and call destMethod(dest, orig, extra) instead of dest.Method(orig, extra) throughout

// Most transports can remain unchanged

//// storage/store_image.go
func (s *storageImageDestination) MethodWithExtra(…) ReturnType {
    // actual implementation
}
func (s *storageImageDestination) Method(OriginalParams) {
    // ⚠️ 
    return ReturnType(errors.New("storage Method no longer supported, MethodWithExtra must be used by caller"))
}

but

• arguably storageImageDestination no longer actually implements ImageDestination if it just fails like that.
• practically speaking, buildah/pkg/blobcache.blobCacheDestination, until it is extended to also support ImageDestinationExt42, will break when interposed in front of c/image/storage.
  • (In some cases, this could be prevented by passing an extensible struct as one of OriginalParams, so that copy could add extra values that blobcache could pass through without being aware of them, but in general that doesn’t work if blobcache needs to update the extra values to be consistent with the rest of its operation instead of passing them through unmodified.)

OTOH, if a particular transport has existed before ImageDestinationExt42 existed, it was possible to implement it without that information, so it must be possible, in the worst case, to fall back to the pre-Ext42 implementation, and work fine for pre-Ext42 callers with pre-Ext42 information – “only” at the cost of maintaining fallback code that is not too likely to be exercised/tested.

But then again, if we introduced Ext42 to support a new transport that never existed before Ext42, there may not exist any reasonable way to implement that transport without the Ext42 information. Now, was that an API break for pre-Ext42 callers (because a declared API now breaks, and they can get such values through alltransports.ParseImageName) or not (because the new transport was never previously available to them)? I guess, that would be a legitimate reason for declaring a new major version, anyway.

---

I guess one different way to think about this is that we want a different lifecycle, and versioning, for some of the interfaces like ImageDestination, where it is very convenient to be able to move them quickly, without breaking APIs of some other interfaces like copy.Image, where it is very desirable to keep the API stable.

So, maybe two different Go modules in a single repo? That seems to be possible but with various gotchas, based on quick searches. But then again, that would not help Buildah, which, via blobcache, depends on the quick-moving interfaces, much.

[mtrmac]

… or should we just go all the way and declare ImageDestination (and even ImageSource? Notably more disruptive) internal, move blobcache and everything like that inside this repository, and maybe provide a non-interface stable ImageSource-like interface for read-only consumers?

[vrothberg]

… or should we just go all the way and declare ImageDestination (and even ImageSource? Notably more disruptive) internal, move blobcache and everything like that inside this repository, and maybe provide a non-interface stable ImageSource-like interface for read-only consumers?

💯, assuming we can find a public abstraction for Buildah to use the cache. Having those things internal would let us move faster and, maybe, sleep better :)

[mtrmac]

I think at this point we have no appetite for moving to c/image/v6; the research in #1439 and the following merged changes to move a lot of the interfaces to c/image/internal/private has alleviated the primary pain points that would motivate that.