Unify naming, improve readability and code correctness

[msehnout]

Inconsistent naming

The current implementation refers to the same variable with 3 different names:

1. composeType

   osbuild-composer/internal/store/store.go

   Line 446 in 02a194f

   func (s *Store) PushCompose(composeID uuid.UUID, bp *blueprint.Blueprint, checksums map[string]string, arch, composeType string, uploadTarget *target.Target) error {

2. OutputType

   osbuild-composer/internal/store/store.go

   Line 475 in 02a194f

   OutputType: composeType,

3. outputFormat

   osbuild-composer/internal/distro/distro.go

   Line 37 in 02a194f

   Pipeline(b *blueprint.Blueprint, additionalRepos []rpmmd.RepoConfig, checksums map[string]string, outputArchitecture, outputFormat string) (*pipeline.Pipeline, error)

Undefined terminology

We should start using "type" or "format" consistently across the codebase and also define our terminology, preferably in some guide for contributors:

• type vs format
• output vs compose
• job vs build
• probably some more

Weak typing

Also I would prefer to use enumeration for architectures and compose types instead of a string which is error prone. For example:

// Not necessarily valid Go code
type Architecture int

const (
        X86_64 Base = iota
        AARCH64
        PPC64
)

// helper function as an input validation
func ArchitectureFromString(input string) Architecture { 
    // ...
}

[teg]

I agree. Thanks for writing this up!

Enums over strings seems like a no-brainer to me, so no further comments on that.

Naming:

• I think we should use the term "Job" over "Build" (just makes sense to me that a "worker" does a "job").
• The weldr API uses ComposeType, we may want to do the same. Though we should check what the UI does/wants to do, and consider following that instead if different (@jgiardino, thoughts?)? Also, we may end up wanting to have several outputs, with different types for a given compose (azure+aws+...), and then referring to this as ComposeType might be wrong.

[jgiardino]

Here is an example from a design document I put together last year. This example shows how we could present the hierarchy of a compose, and all the images/jobs created from that compose, and then the uploads per image.

After doing a few rounds of reviews, the terminology I ended up using is the following. But please let me know if you have suggestions.

• Compose
• Image build
• Upload action

[msehnout]

These two answers seems to be in contradiction, right?
1.

After doing a few rounds of reviews, the terminology I ended up using is the following. But please let me know if you have suggestions.

* Compose

* Image build

* Upload action

2.

Naming:

* I think we should use the term "Job" over "Build" (just makes sense to me that a "worker" does a "job").

---

My formal proposal (I think we could include it in the README once we agree on it)

Compose

• Compose is what the user submits over one of the frontends
• It contains of one or more image builds
• It contains zero or more upload actions

Image build

(I'm using the terminology from @jgiardino so feel free to disagree @teg )

• Is a unit of work performed by osbuild (internally it is a single execution of osbuild)
• The resulting image has a format:
  https://github.com/osbuild/osbuild-composer/blob/master/internal/pipeline/qemu_assembler.go#L12
  https://github.com/osbuild/osbuild/blob/master/assemblers/org.osbuild.qemu#L539

Image format

• Can be qcow2, vhd, vmdk, etc.
• In the cockpit-composer it maps to more human-readable names such as "Azure image (vhd)"

Upload action

• Each image can be, but does not have to be, uploaded to a remote location
• One image can be uploaded to multiple locations (can it?)

---

Proposed code change:

-  func (s *Store) PushCompose(composeID uuid.UUID, bp *blueprint.Blueprint, checksums map[string]string, arch, composeType string, uploadTarget *target.Target) error { 
+  func (s *Store) PushCompose(composeID uuid.UUID, bp *blueprint.Blueprint, checksums map[string]string, arch, imageFormats []ImageFormat, uploadTarget *target.Target) error { 

where ImageFormat is an enumeration.

[jgiardino]

• In the context of osbuild or the worker, I don't have a strong opinion on whether the process is called a job or an image build. In the context of the UI, however, job is too generic for us to use there.
• In the terminology defined above, should image format be replaced with image type? The user specifies the image type, which determines what the image format will be. I consider the image format to be the file format of the image file that's created. Additionally:
  • There can be some formats that are used by more than one image type (e.g. qcow2)
  • In the future, could there be some types that produce images of more than one format?

[ondrejbudai]

I agree with Jenn, image type is imho more suitable than image format - the example with qcow2 s great.

[msehnout]

* In the context of osbuild or the worker, I don't have a strong opinion on whether the process is called a **job** or an **image build**. In the context of the UI, however, **job** is too generic for us to use there.

Right, we distinguish between pending "build" and a running one, which we call a "job".

* In the terminology defined above, should **image format** be replaced with **image type**? The user specifies the **image type**, which determines what the **image format** will be. I consider the image format to be the file format of the image file that's created. Additionally:
  
  * There can be some formats that are used by more than one image type (e.g. qcow2)
  * In the future, could there be some types that produce images of more than one format?

You are right, so we should use "image type" in the API I mention above. But we might want to reconsider naming of the types we use now because, as you mention, we use: ami, qcow2, openstack, vhd, etc. but what we really implement is: AWS image, generic qcow2, OpenStack image, Azure image.

[msehnout]

Edited version from my comment above ^^^

My formal proposal (I think we could include it in the README once we agree on it)

Compose

• Compose is what the user submits over one of the frontends
• It contains of one or more image builds
• It contains zero or more upload actions

Image build

• Is a unit of work performed by osbuild (internally it is a single execution of osbuild)
• The resulting image has a type:
  https://github.com/osbuild/osbuild-composer/blob/master/internal/distro/fedora30/distro.go#L19
• Running build in osbuild-composer is referred to as a "job" (internal terminology, not related to end-user experience)

Image type

• In the cockpit-composer these are for example:
  • Openstack
  • Azure
  • AWS
• Internally we name them by their file format: vhd, ami, etc., but we might reconsider this as different types can result in the same file format, but with different content (e.g. packages installed).

Upload action

• Each image can be, but does not have to be, uploaded to a remote location
• One image can be uploaded to multiple locations (can it?)

---

Do we want to replace the variable name "output" and use "image" instead? In this context we might want to use "types" or something like that to better reflect the newly proposed terminology.
https://github.com/osbuild/osbuild-composer/blob/master/internal/distro/fedora30/distro.go#L19

[ondrejbudai]

But we might want to reconsider naming of the types we use now because, as you mention, we use: ami, qcow2, openstack, vhd, etc. but what we really implement is: AWS image, generic qcow2, OpenStack image, Azure image.

I think that's a good idea. Unfortunately, I'm a bit afraid of backwards compatibility. I think cockpit-composer checks for available image types but the API tests have hardcoded values.

[teg]

These two answers seems to be in contradiction, right?

Yes, and no. Under normal circumstances, I would now challenge Jenn to a round of jousting, but I think we may actually want both things: I agree with all the definitions from Martin's last comment, but I would also add:

Job

• What composer submits to a worker
• Consists of ONE image build
• And ONE OR MORE Upload Actions

This concept would not appear in the UI though, composer would take the Compose and split it into several Jobs, one per Image build (with potentially several Upload actions attached to each).

[jgiardino]

Yes, and no. Under normal circumstances, I would now challenge Jenn to a round of jousting, but I think we may actually want both things

Yes, I like keeping both image build and job as defined in the previous comments.

Even so, I would still be down for a team outing to an renaissance faire next week.

[msehnout]

I think most of these issues have been addressed so I'm closing this now. Follow up issue is: #200