UICatalogEntry pruning ALM-438 #269
Conversation
|
I have a work-in-progress branch for Console that eliminates the need for |
|
@alecmerdler awesome, let's keep this around just in case, and we'll close it when/if the console branch is merged |
|
This has some catalog fixes that we'll need to include even if we're not using uicatalogentries |
ecordell
force-pushed
the
uicatalog-cleanup
branch
from
28ddc65
to
ea2ee88
Compare
| @@ -40,7 +41,7 @@ var installPlanFailedChecker = func(fip *installplanv1alpha1.InstallPlan) bool { | |||
| return fip.Status.Phase == installplanv1alpha1.InstallPlanPhaseFailed | |||
| } | |||
|
|
|||
| func FetchUICatalogEntries(t *testing.T, c opClient.Interface, count int) (*opClient.CustomResourceList, error) { | |||
| func fetchUICatalogEntries(t *testing.T, c opClient.Interface, count int) (*opClient.CustomResourceList, error) { | |||
e2e/alm_e2e_test.go
Outdated
| c := newKubeClient(t) | ||
|
|
||
| // Load old configmap (contains packages A and B) | ||
| oldFile, err := os.Open("e2e/data/catalog.old.yaml") |
There was a problem hiding this comment.
Should we make this an absolute path, or explicitly relative?
| _, err = fetchUICatalogEntry(t, c, "package-c") | ||
| require.NoError(t, err) | ||
|
|
||
| // B should've been updated |
There was a problem hiding this comment.
Nit: it's hard to tell where set up ends and tests begin. Maybe comments delineating?
pkg/client/uicatalogentry_client.go
Outdated
| @@ -17,6 +18,8 @@ import ( | |||
|
|
|||
| type UICatalogEntryInterface interface { | |||
| UpdateEntry(csv *v1alpha1.UICatalogEntry) (*v1alpha1.UICatalogEntry, error) | |||
| ListEntries(namespace string) (*v1alpha1.UICatalogEntryList, error) | |||
| Delete(name, namespace string, options *metav1.DeleteOptions) error | |||
There was a problem hiding this comment.
Most places have the order namespace, name. Can we keep it consistent by changing the order here?
| // Sync creates UICatalogEntry CRDs for each package in the catalog. Fails immediately on error. | ||
| func (store *CustomResourceCatalogStore) Sync(catalog Source) ([]*v1alpha1.UICatalogEntry, error) { | ||
| // Sync creates UICatalogEntry CRDs for each package in the catalog and removes old ones. Fails immediately on error. | ||
| func (store *CustomResourceCatalogStore) Sync(catalog Source, source *catalogv1alpha1.CatalogSource) ([]*v1alpha1.UICatalogEntry, error) { |
There was a problem hiding this comment.
how about passing in owners []metav1.OwnerReference instead of source to keep it more generic and clean? It'd remove the need to import catalogv1alpha1 as well.
| existingMap[existing.Name] = existing | ||
| } | ||
|
|
||
| // prune things not controlled by any source |
There was a problem hiding this comment.
do we? seems like unexpected behavior if you manually create one.
There was a problem hiding this comment.
when we upgrade from tectonic helium, all existing uicatalogentries won't have any owners, so this will clean them up and replace with owned versions.
pkg/registry/uicatalogentries.go
Outdated
| } | ||
| } | ||
| for _, new := range newEntries { | ||
| newMap[new.Name] = new |
There was a problem hiding this comment.
nit: since new is a keyword (as is new(map[string]string)), even though it's not showing an error, can it be renamed to something else?
ecordell
force-pushed
the
uicatalog-cleanup
branch
from
ea2ee88
to
3ea6779
Compare
ecordell
force-pushed
the
uicatalog-cleanup
branch
from
3ea6779
to
ea969e9
Compare
…anup UICatalogEntry pruning ALM-438
…anup UICatalogEntry pruning ALM-438
This adds pruning logic for UICatalogEntries