Cleanup usage of Image struct in Build and BuildRun

[SaschaSchwarze0]

In pull request Specify annotations and labels to be set on output images #854, the Image struct was extended with the annotations and labels properties.

Those values are only relevant for the Build's spec.output, but the type is also used for the Build's spec.builderImage where those fields are not relevant. It is also used in the BuildRun's spec.output.

I suggest the following two changes:

• We officially support labels and annotations in the BuildRun's spec.output. All other properties in the BuildRun's spec.output overwrite the Build's spec.output (the image). From a scenario perspective it would here actually make more sense to merge the maps with precedence for the BuildRun settings. So, that part is worth a quick discussion.
• We use a different type for the Build's spec.builderImage that does not have the two properties.

Edit by @adambkaplan: removing fields is a destructive API change, we should consider deprecating the field entirely in #935

[adambkaplan]

Removing the "good first issue" label.

@SaschaSchwarze0 hadn't we discussed removing the spec.builderImage field entirely? The source-to-image build strategies are the only thing which uses this fiedl IIRC, and with the Parameters enhancement we can use that instead.

[adambkaplan]

Our current code hasn't broadcast that spec.builder (which is the YAML/JSON representation) is deprecated. This would warrant a SHIP so that we plan accordingly.

In the interim the current API can be captured in a separate Golang object so that spec.output can evolve independently.

[adambkaplan]

Opened #935 to capture the deprecation of spec.builder

[sm43]

/assign

@adambkaplan @SaschaSchwarze0 just want to clarify with which approach do we wanna proceed?

(from the description)
We officially support labels and annotations in the BuildRun's spec.output. All other properties in the BuildRun's spec.output overwrite the Build's spec.output (the image). From a scenario perspective it would here actually make more sense to merge the maps with precedence for the BuildRun settings

or

(from the comment)
In the interim the current API can be captured in a separate Golang object so that spec.output can evolve independently.

we can support labels and annoatation for buildrun by using the same Image struct for both build and buildrun
and for spec.builder we can add new struct with existing fields till we deprecate it.
WDYT?

[SaschaSchwarze0]

Hi @sm43,

I think there are two open questions:

(1) How do we handle the structs?

Here I think @adambkaplan and myself have a common idea which is to continue to use Image for the output in Build and BuildRun, but to introduce something new for the builder image (such as BuilderImage) without the annotations and labels.

(2) Does the BuildRun-defined output completely override the Build-defined output, or do we merge things?

I tend to merge. Which means:

kind: Build
spec:
  output:
    image: myimage
    credentials:
      name: mysecret
    annotations:
      anAnnotation1: aValue1
      anAnnotation2: aValue2
   labels:
      aLabel1: aValue1

and

kind: BuildRun
spec:
  output:
    annotations:
      anAnnotation2: newValue

would (a) be allowed (= specifying an output in a BuildRun but omit the image) and (b) lead to those annotations and labels:

• Annotations: anAnnotation1: aValue1, anAnnotation2: newValue
• Labels: aLabel1: aValue1

[gabemontero]

Hi @sm43,

I think there are two open questions:

(1) How do we handle the structs?

Here I think @adambkaplan and myself have a common idea which is to continue to use Image for the output in Build and BuildRun, but to introduce something new for the builder image (such as BuilderImage) without the annotations and labels.

per @adambkaplan 's #922 (comment) in this issue
we are actually talking about deprecating the builder image field: #935

[sm43]

Hi @sm43,
I think there are two open questions:
(1) How do we handle the structs?
Here I think @adambkaplan and myself have a common idea which is to continue to use Image for the output in Build and BuildRun, but to introduce something new for the builder image (such as BuilderImage) without the annotations and labels.

per @adambkaplan 's #922 (comment) in this issue we are actually talking about deprecating the builder image field: #935

@gabemontero I think we want define a new struct for BuilderImage because there are labels and annotation field in Image struct which we don't require for it.
So, what if we define a new struct BuilderImage without those 2 fields for now and the deprecate as per the process (#935)
wdyt?

[gabemontero]

Hi @sm43,
I think there are two open questions:
(1) How do we handle the structs?
Here I think @adambkaplan and myself have a common idea which is to continue to use Image for the output in Build and BuildRun, but to introduce something new for the builder image (such as BuilderImage) without the annotations and labels.

per @adambkaplan 's #922 (comment) in this issue we are actually talking about deprecating the builder image field: #935

@gabemontero I think we want define a new struct for BuilderImage because there are labels and annotation field in Image struct which we don't require for it. So, what if we define a new struct BuilderImage without those 2 fields for now and the deprecate as per the process (#935) wdyt?

Ignoring applying this issue's changes to BuilderImage since plan on deprecating it is fine for me.

But to be clear, that is different than defining a new "temporary" struct.

We should have a note in the doc that accompanies the implementation of the issue that explains why it was ignored (in case users noticed the discrepancy and wonder why). We can then remove that note when we actually remove the field.

[adambkaplan]

Closing this issue as done, completed in v0.7.0. The portion related to deprecating/removing spec.builder is addressed in #935.