Add configurable storage class name for image resource

[tylerphelan]

• If set, will be used as the storageClassName in the pvc

[jjbustamante]

It looks good to me!

[tomkennedy513]

I'm wondering if we actually need to validate that the storage class exists since that will require additional rbac for our controller. If we create a pod with an invalid storage class can't we just let pod fail?

[tomkennedy513]

This is awesome!

[matthewmcnew]

Does this need to be supported in v1alpha1 apis? #934 (comment)

[tylerphelan]

@matthewmcnew

Does this need to be supported in v1alpha1 apis? #934 (comment)

Let's think of the scenarios:

converting v1alpha1 -> v1alpha2:
storageClassName is always "", v1alpha2 clients will use ""

converting v1alpha2 -> v1alpha1:
v1alpha1 client (old version of kp) patches existing v1alpha2 image, causing storageClassName to be set to "" which will violate the immutability constraint causing blocking error. This is not great.

My 2c is that if an old v1alpha1 client runs into an immutability error it will not lose data. If the storageClassName is unset in v1alpha2 behavior will not be affected by v1alpha1 clients. Users should upgrade kp.

[matthewmcnew]

v1alpha1 client (old version of kp) patches existing v1alpha2 image, causing storageClassName to be set to "" which will violate the immutability constraint causing blocking error. This is not great.

This seems rare but, not great. It essentially means we are not supporting v1alpha1.

Users should upgrade kp.

If our strategy for v1alpha1 headaches is to say not use v1alpha1 we should probably just begin removing v1alpha1.

[tylerphelan]

It essentially means we are not supporting v1alpha1.

risk seems very high

If our strategy for v1alpha1 headaches is to say not use v1alpha1 we should probably just begin removing v1alpha1.

If we don't do this will we need to migrate all new fields to prevent data loss? ex DefaultProcess, ProjectDescriptorPath, CreationTime ?

[matthewmcnew]

If we don't do this will we need to migrate all new fields to prevent data loss? ex DefaultProcess, ProjectDescriptorPath, CreationTime

We probably should provide support for these. Hitting an expected immutability constraint seems slightly worse to me.

[tylerphelan]

Hitting an expected immutability constraint seems slightly worse to me.

I think data loss is equal if not worse

[tylerphelan]

I think we should probably add migrations

[matthewmcnew]

I think we should probably add migrations

Do you think we should do that as this PR or create an issue to support it for all susceptible fields?

[tylerphelan]

I've added the conversion for this one here and have added #1036

[codecov-commenter]

Codecov Report

Merging #1029 (425de88) into main (e9d424b) will decrease coverage by 0.06%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##             main    #1029      +/-   ##
==========================================
- Coverage   70.53%   70.47%   -0.07%     
==========================================
  Files         122      122              
  Lines        5726     5740      +14     
==========================================
+ Hits         4039     4045       +6     
- Misses       1313     1319       +6     
- Partials      374      376       +2     

Impacted Files	Coverage Δ
pkg/apis/build/v1alpha2/image_builds.go	69.49% <0.00%> (-1.61%)	⬇️
pkg/apis/build/v1alpha2/image_types.go	50.00% <ø> (ø)
pkg/cnb/env_vars.go	86.36% <ø> (ø)
pkg/apis/build/v1alpha2/image_conversion.go	85.26% <57.14%> (-3.11%)	⬇️
pkg/apis/build/v1alpha2/image_validation.go	98.21% <100.00%> (+0.01%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[tylerphelan]

will merge after 0.7.1 is released