Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Add command to delete an existing olmv1 catalog #219

Merged

Conversation

azych
Copy link
Contributor

@azych azych commented Feb 26, 2025

This adds delete command that enables deleting either a single or all existing catalogs (ClusterCatalog) from the cluster.

Signature:

Delete either a single or all of the existing catalogs

Usage:
  operator olmv1 delete catalog [catalog_name] [flags]

Aliases:
  catalog, catalogs [catalog_name]

Flags:
      --all    delete all catalogs
  -h, --help   help for catalog

Example walkthrough:

➜  kubectl get clustercatalogs
NAME            LASTUNPACKED   SERVING   AGE
operatorhubio   5m41s          True      5m51s
➜ go run *.go olmv1 delete catalog unknown
failed to delete catalog "unknown": clustercatalogs.olm.operatorframework.io "unknown" not found
exit status 1
➜ go run *.go olmv1 delete catalog operatorhubio
catalog "operatorhubio" deleted
➜ kubectl get clustercatalogs
No resources found

➜ kubectl get clustercatalogs
NAME             LASTUNPACKED   SERVING   AGE
operatorhubio    2m4s           True      2m11s
operatorhubio2   5s             True      18s
➜ go run *.go olmv1 delete catalog --all
catalog "operatorhubio" deleted
catalog "operatorhubio2" deleted
➜ kubectl get clustercatalogs
No resources found

➜ kubectl get clustercatalogs
NAME             LASTUNPACKED   SERVING   AGE
operatorhubio    55s            True      62s
operatorhubio2   45s            True      59s
➜ kubectl scale deploy catalogd-controller-manager operator-controller-controller-manager --replicas=0 -n olmv1-system
➜ go run *.go olmv1 delete catalog --all
failed deleting catalogs: failed deleting catalog "operatorhubio": waiting for deletion: context deadline exceeded
failed deleting catalog "operatorhubio2": client rate limiter Wait returned an error: context deadline exceeded
exit status 1

➜ kubectl get clustercatalogs
NAME             LASTUNPACKED   SERVING   AGE
operatorhubio2   23s            True      30s
➜ go run *.go olmv1 delete catalog operatorhubio2 --all
failed to delete catalog "operatorhubio2": name cannot be provided when a selector is specified
exit status 1
➜ go run *.go olmv1 delete catalog --all operatorhubio2
failed to delete catalog "operatorhubio2": name cannot be provided when a selector is specified
exit status 1

Additionally, does a light refactor of the old helpers, especially the waitForDeletion, which does not accept variable number of input objects anymore.
Benefits of this are:

  • callers have more control over deletion and can decide how they want to run it - async/sync for example
  • de-duplicated error messages - no need to mention details of the object within the helper
  • no need for extending the deleting object with GVK info just for the sake of helper and its logging

There is some overlap with #218 - ie. test helpers, which will be cleaned up once that has been merged - done

Part of operator-framework/operator-controller#1815 ; operator-framework/operator-controller#1770

Copy link
Contributor

@ankitathomas ankitathomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2025
@azych azych force-pushed the olmv1-delete-catalog branch from 85a9c09 to d30d85c Compare March 4, 2025 10:46
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2025
@azych azych force-pushed the olmv1-delete-catalog branch 2 times, most recently from 2490933 to fde6841 Compare March 5, 2025 10:40
@LalatenduMohanty
Copy link
Member

Currently the command is

$ kubectl operator olmv1 delete  catalog operatorhubio
catalog "operatorhubio" deleted

And I think is too long. I would love it to be as $ kubectl operator olmv1 delete --catalog <name of the catalog or catalogs>

$ kubectl operator olmv1 delete --catalog operatorhubio

WDYT?

@LalatenduMohanty
Copy link
Member

I am wondering if we should add an flag --all to kubectl operator olmv1 delete catalog to delete all the catalogs. We can do it in another PR if we think it will help, but it is not very important or urgent right now.

$ kubectl operator olmv1 delete catalog
failed deleting catalogs: resource name may not be empty

@azych
Copy link
Contributor Author

azych commented Mar 6, 2025

Currently the command is

$ kubectl operator olmv1 delete  catalog operatorhubio
catalog "operatorhubio" deleted

And I think is too long. I would love it to be as $ kubectl operator olmv1 delete --catalog <name of the catalog or catalogs>

$ kubectl operator olmv1 delete --catalog operatorhubio

WDYT?

Your idea to change the type to be a flag on the command makes the overall command longer.

Overall I think we should continue with the syntax we agreed on and used so far. If you feel strongly about this, maybe olmv0 deprecation might be a good occasion to propose a new syntax idea? As part of olmv0 deprecation we might modify the current command 'prefix' - `operator olmv1'.

Hope that's fair.

@azych
Copy link
Contributor Author

azych commented Mar 6, 2025

I am wondering if we should add an flag --all to kubectl operator olmv1 delete catalog to delete all the catalogs. We can do it in another PR if we think it will help, but it is not very important or urgent right now.

$ kubectl operator olmv1 delete catalog
failed deleting catalogs: resource name may not be empty

This PR already includes --all

@azych azych force-pushed the olmv1-delete-catalog branch from fde6841 to 27efe6e Compare March 6, 2025 09:27
@ankitathomas
Copy link
Contributor

I am wondering if we should add an flag --all to kubectl operator olmv1 delete catalog to delete all the catalogs. We can do it in another PR if we think it will help, but it is not very important or urgent right now.

$ kubectl operator olmv1 delete catalog
failed deleting catalogs: resource name may not be empty

This PR already includes --all

@LalatenduMohanty do you mean treating olmv1 delete catalog --all as successful when there are no clustercatalogs to delete?

@LalatenduMohanty
Copy link
Member

LalatenduMohanty commented Mar 6, 2025

This PR already includes --all

I did not see the --all when I am testing the CLI, my bad. Let me check again.

Copy link
Member

@LalatenduMohanty LalatenduMohanty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2025
@LalatenduMohanty LalatenduMohanty added this pull request to the merge queue Mar 6, 2025
Merged via the queue into operator-framework:main with commit 026eb84 Mar 6, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants