Have the ClusterBuildTemplate controller instantiate Image resources. #354
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
mattmoor
force-pushed
the
cbt-images
branch
from
d17d6b9 to
4a076df
Compare
| } | ||
|
|
||
| // Delete any Image caches relevant to older versions of this resource. | ||
| propPolicy := metav1.DeletePropagationForeground |
There was a problem hiding this comment.
Can you add a test that cached images are deleted upon template update?
| if missing := allCached(ics, eics); len(missing) > 0 { | ||
| grp, _ := errgroup.WithContext(ctx) | ||
|
|
||
| for _, m := range missing { |
There was a problem hiding this comment.
Can you add a test that new images are cached upon template update?
| return nil | ||
| } | ||
|
|
||
| func (c *Reconciler) reconcileImageCaches(ctx context.Context, cbt *v1alpha1.ClusterBuildTemplate) error { |
There was a problem hiding this comment.
Can this be moved to a common location shared by both types of template reconcilers?
There was a problem hiding this comment.
I've made the parts that are common usable from the buildtemplate reconciler.
| caching "github.com/knative/caching/pkg/apis/caching/v1alpha1" | ||
| ) | ||
|
|
||
| func TestMakeImageCache(t *testing.T) { |
There was a problem hiding this comment.
Duplicating this test also feels Wrong, if the code was in a single common location then you'd only need to have one test for it.
There was a problem hiding this comment.
This checks that we properly create things in the system namespace. I can reduce the testing, but the vast majority of the logic is shared :)
There was a problem hiding this comment.
Yeah I think what I'm asking for is a test that does all of these cases, for either BuildTemplate or ClusterBuildTemplate, and checks that the resources are created either in the template's namespace or in the case of ClusterBuildTemplate, the system namespace. Sort of like a two-level nested table-driven test, does that make sense?
There was a problem hiding this comment.
Yes, let me spend some time on this. Will ping this when RFAL.
There was a problem hiding this comment.
ok this should be RFAL.
mattmoor
force-pushed
the
cbt-images
branch
from
4a076df to
dbc96ee
Compare
mattmoor
force-pushed
the
cbt-images
branch
from
dbc96ee to
09230b2
Compare
mattmoor
changed the title
| @@ -193,3 +182,18 @@ func allCached(desired []caching.Image, observed []*caching.Image) (missing []ca | |||
| } | |||
| return missing | |||
| } | |||
|
|
|||
| func CreateMissingImageCaches(ctx context.Context, client cachingclientset.Interface, missing []caching.Image) error { | |||
There was a problem hiding this comment.
Could this just transparently calculate missingImageCaches and create them? Users shouldn't need to call both methods.
There was a problem hiding this comment.
I changed this so that CreateMissingImageCaches calls missingImageCaches.
| } | ||
|
|
||
| // Make sure we have all of the desired caching resources. | ||
| if missing := buildtemplate.MissingImageCaches(ics, eics); len(missing) > 0 { |
There was a problem hiding this comment.
Taking this even further, buildtemplate.ReconcileImageCaches could:
- Calculate missing images
- Cache missing images
- Uncache old images
There was a problem hiding this comment.
I think I like the split we reach above since the creates can be done in a generic way without the namespace coming into play. There's already a large number of arguments, and I generally prefer largely single-purpose functions.
| caching "github.com/knative/caching/pkg/apis/caching/v1alpha1" | ||
| ) | ||
|
|
||
| func TestMakeImageCache(t *testing.T) { |
There was a problem hiding this comment.
Yeah I think what I'm asking for is a test that does all of these cases, for either BuildTemplate or ClusterBuildTemplate, and checks that the resources are created either in the template's namespace or in the case of ClusterBuildTemplate, the system namespace. Sort of like a two-level nested table-driven test, does that make sense?
mattmoor
force-pushed
the
cbt-images
branch
from
09230b2 to
16a016b
Compare
Unlike the BuildTemplate controller, which colocates the Image references, this places the Image resources (which are namespaced) in the knative-build namespace.
mattmoor
force-pushed
the
cbt-images
branch
from
16a016b to
2fed84a
Compare
|
The following is the coverage report on pkg/.
|
…knative#354) Unlike the BuildTemplate controller, which colocates the Image references, this places the Image resources (which are namespaced) in the knative-build namespace.
…knative#354) Unlike the BuildTemplate controller, which colocates the Image references, this places the Image resources (which are namespaced) in the knative-build namespace.
Unlike the BuildTemplate controller, which colocates the Image references, this places the Image resources (which are namespaced) in the knative-build namespace.
WIP until #353 lands.