Clean up behaviour of ostree.Resolve()

[achilleas-k]

images/pkg/ostree/ostree.go

Lines 218 to 259 in 711ff22

	// Resolve the ostree source specification to a commit specification.
	//
	// If a URL is defined in the source specification, the checksum of the ref is
	// resolved, otherwise the checksum is an empty string. Failure to resolve the
	// checksum results in a ResolveRefError.
	//
	// If the ref is already a checksum (64 alphanumeric characters), it is not
	// resolved or checked against the repository.
	//
	// If the ref is malformed, the function returns with a RefError.
	func Resolve(source SourceSpec) (CommitSpec, error) {
	commit := CommitSpec{
	Ref: source.Ref,
	URL: source.URL,
	}
	if source.RHSM {
	commit.Secrets = "org.osbuild.rhsm.consumer"
	}
	if verifyChecksum(source.Ref) {
	// the ref is a commit: return as is
	commit.Checksum = source.Ref
	return commit, nil
	}
	if !verifyRef(source.Ref) {
	// the ref is not a commit and it's also an invalid ref
	return CommitSpec{}, NewRefError("Invalid ostree ref or commit %q", source.Ref)
	}
	// URL set: Resolve checksum
	if source.URL != "" {
	// If a URL is specified, we need to fetch the commit at the URL.
	checksum, err := ResolveRef(source.URL, source.Ref, source.RHSM, nil, nil)
	if err != nil {
	return CommitSpec{}, err // ResolveRefError
	}
	commit.Checksum = checksum
	}
	return commit, nil
	}

While writing the otk container resolver and discussing certain details around testing with @mvo5, I realised that I'm not really happy with some of the behaviour of this function.

Some things to reconsider:

• The checksum passthrough behaviour is a bit strange (it makes the resolver a no-op) but I think it's useful for throwing user input at the function and knowing that what you get back is a commit ID that will work. I don't want to remove the ability to pass a checksum instead of a ref to the function, but maybe we can verify that the checksum ID exists in the repo at the URL.
• The URL == "" behaviour even stranger. What's the point of calling a resolver when there's nothing to resolve? I think this is left over from older behaviour which was also doing things with the parent ref and shuffling things around. I think it would make more sense if URL == "" was an error condition.
• The interaction with the ResolveRef() function in the same file is a bit bizarre. The ResolveRef() function is never called from anywhere else, not even osbuild-composer, and yet we have two arguments that are always nil. ResolveRef() is the function that's actually doing the resolving (of the ref), whereas Resolve() is meant to resolve arguments, figure out if a call to the URL is needed etc. Perhaps they should be merged, making ostree.Resolve() the only entrypoint into the resolver and putting some work onto the caller (like not bothering with the function call when the URL is empty and no resolve is desired).