[RFC] cloud: add AwsUploader.Result() with structured info

[mvo5]

[draft: needs tests and discussion, could also wait until we have a GCP uploader so that we know if this translates to that reasonablly well]

This commit adds a new AwsUploader.Result() call that allows to get structured information about the upload result. This is needed for e.g. osbuild-composer.

It is not part of the UploadAndRegister() call because the exact information about the result is cloud specific. Having a common uploader interface is nice as it simplifies/unifies the common code paths (eg. [0]). So this tries to be find the right balance. Just like each uploader will have a different NewUploader() with different arguments the Result() will also be slightly different for them all.

[0] https://github.com/osbuild/image-builder-cli/blob/main/cmd/image-builder/upload.go#L36

[thozza]

Thanks.

I was thinking more along the lines of extending the Uploader interface either by returning the results from the UploadAndRegister or adding a Result() method to the interface. That has the downside of requiring the method signature to be the same for all cloud-specific Uploaders.

While the implied expectation is probably that the Uploader instance will be for single use, nothing prevents one from using it more than once. The action may fail if the image with the given name already exists (I'm not sure if this is a problem since, in AWS, the name is just a tag). If it does not, the AMI in the result would be overwritten.

I quickly drafted my idea so that we also have a different version. And yeah, it is similar to how we handled target results in composer, but I found that approach reasonably clean.

[mvo5]

Thanks.

I was thinking more along the lines of extending the Uploader interface either by returning the results from the UploadAndRegister or adding a Result() method to the interface. That has the downside of requiring the method signature to be the same for all cloud-specific Uploaders.

Yeah, its an interessting problem. I think in practice by returning the result from UploaderAndRegister() we will return an interface{} as the (cloud specific) Result and then the caller needs to cast it (sadly go makes this hard). The caller knows what cloud it used so that is fine. But it seems equivalent to having to call a (cloud specific) Result() method that will give us a compile time type-safe result as the benefit (and arguably we already need cloud specific NewUploader() so this is not fully abstracted already anyway).

While the implied expectation is probably that the Uploader instance will be for single use, nothing prevents one from using it more than once. The action may fail if the image with the given name already exists (I'm not sure if this is a problem since, in AWS, the name is just a tag). If it does not, the AMI in the result would be overwritten.

I agree that its not super nice, but maybe not such a big deal, Result() would simply the last result, we could even rename it accordingly. We could of course also change the code to prevent multiple uploads but that seems not necessary.

I quickly drafted my idea so that we also have a different version. And yeah, it is similar to how we handled target results in composer, but I found that approach reasonably clean.

I am not a fan of the IsUploadResult pattern (at all), if we go this route I would really like to avoid using them (but then maybe I'm missing something about the pattern).

[thozza]

I've created https://github.com/orgs/osbuild/discussions/43 to discuss this.

[lzap]

As I commented in the discussion, I believe that this generalization should not be done this way. It exposes the data directly, what this should return instead is an interface that can be rendered (shown to the user) in various ways. Data can be either pulled out of it via getter, or pushed like a formatter that injects data into a common structure. Example of a pull approach that could be suitable for a CLI caller:

type Value struct {
 K string
 V string
}

type ValuesResult interface {
 Values() []Value
}

Or push approach:

type ResultWriter interface {
 WriteResults(io.Writer)
}

Where results are literally written as text/json or whatever.

[mvo5]

It seems we are not really reaching consensus just yet, what we could do is add one (or two) more clouds (gcp first) and apply what we learned and get back to this discussion?

[croissanne]

I'd be interested to see another cloud or two. But if it's just to pass around within go code (not being serialised), I'm erring on the side of no isFoo interfaces, unless that interface actually has a meaningful abstraction.