Skip to content

Conversation

ankitathomas
Copy link
Contributor

Add documentation to describe current state of olmv1 support in subcommands.

see #236

@openshift-ci openshift-ci bot requested review from jmrodri and joelanford October 1, 2025 15:35
operator olmv1 create [command]

Available Commands:
catalog Create a new catalog
Copy link
Member

Choose a reason for hiding this comment

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

Replacing catalog with ClusterCatalog to keep the naming consistent across the doc

Suggested change
catalog Create a new catalog
catalog Create a new ClusterCatalog

Suggest naming extension to ClusterExtension as well wherever applicable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am keeping the help text consistent for this PR to match the help text that gets generated. There is another issue to track the change #237

Copy link

Choose a reason for hiding this comment

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

Oh, so the actual command help text says "catalog" everywhere right now. That makes sense to me to keep these docs parallel to the current help text and change both the docs and command help text in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apart from this, there are also inconsistencies in flag definitions across commands (--available vs --availability-mode, partial support for flag shorthands between commands, etc.)

Create a new `ClusterCatalog` with the provided name and catalog image reference.

```bash
Create a new catalog
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Create a new catalog
Create a new ClusterCatalog

operator olmv1 delete [command]

Available Commands:
catalog Delete either a single or all of the existing catalogs
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
catalog Delete either a single or all of the existing catalogs
catalog Delete either a single or all of the existing `ClusterCatalogs`

Delete either a single `ClusterCatalog` by name, or all `ClusterCatalogs` on cluster. The `--all` flag allows for cleaning up all existing catalogs on cluster, and cannot be used when a `ClusterCatalog` name is passed as an argument.

```bash
Delete either a single or all of the existing catalogs
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Delete either a single or all of the existing catalogs
Delete either a single or all of the existing `ClusterCatalogs`

catalog, catalogs [catalog_name]

Flags:
--all delete all catalogs
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
--all delete all catalogs
--all delete all `ClusterCatalogs`

operator olmv1 update [command]

Available Commands:
catalog Update a catalog
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
catalog Update a catalog
catalog Update a `ClusterCatalog`

operator olmv1 get [command]

Available Commands:
catalog Display one or many installed catalogs
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
catalog Display one or many installed catalogs
catalog Display one or many installed `ClusterCatalogs`


Available Commands:
catalog Display one or many installed catalogs
extension Display one or many installed extensions
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
extension Display one or many installed extensions
extension Display one or many installed `ClusterExtensions`

Display status information of installed `ClusterExtensions`.

```bash
Display one or many installed extensions
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Display one or many installed extensions
Display one or many installed `ClusterExtensions`

Copy link
Member

@rashmigottipati rashmigottipati left a comment

Choose a reason for hiding this comment

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

Looks great overall.

(optional): A couple of suggestions:

  • It'd be nice to add one example for each subcommand so it's easier for users to understand expected usage quickly.
  • Add a short description for ClusterCatalog and ClusterExtension before linking to the respective APIs

@ankitathomas ankitathomas force-pushed the readme branch 2 times, most recently from d3461a9 to bcbb0a3 Compare October 2, 2025 14:37
Available Commands:
create Create a resource
delete Delete a resource
get Display one or many resource(s)
Copy link

Choose a reason for hiding this comment

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

I think the most idiomatic phrasing here would be "Display one or more resources" or perhaps just "Display resources" which captures both cases

Copy link

Choose a reason for hiding this comment

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

If this is what the actual command help text says though, then disregard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping the comment unresolved since follow up work on improving help text #237 needs to address this.

operator olmv1 create [command]

Available Commands:
catalog Create a new catalog
Copy link

Choose a reason for hiding this comment

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

Oh, so the actual command help text says "catalog" everywhere right now. That makes sense to me to keep these docs parallel to the current help text and change both the docs and command help text in the future.

Flags:
-c, --channels strings channels which would be used for getting updates, e.g, --channels "stable,dev-preview,preview"
-d, --cleanup-timeout duration the amount of time to wait before cancelling cleanup after a failed creation attempt (default 1m0s)
-n, --namespace string namespace to install the operator in
Copy link

Choose a reason for hiding this comment

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

if separate from the cli help text, would reword to something like "installation namespace for the operator" or "destination namespace for the operator installation"

@ankitathomas
Copy link
Contributor Author

* It'd be nice to add one example for each subcommand so it's easier for users to understand expected usage quickly.

Added examples for list to have an example of what the output looks like for get subcommands, but the remaining ones did not have any significant output to show while they run, so it would just be a single line with a command on it. Would this still be worth adding?

@ankitathomas ankitathomas force-pushed the readme branch 4 times, most recently from eaeea71 to deb52e5 Compare October 2, 2025 15:29
Copy link
Member

@rashmigottipati rashmigottipati left a comment

Choose a reason for hiding this comment

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

Can we explicitly mark if there are any required flags for each subcommand?
(even a note like "Required" next to the flag would be fine)

Signed-off-by: Ankita Thomas <[email protected]>
@ankitathomas ankitathomas added this pull request to the merge queue Oct 2, 2025
Merged via the queue into operator-framework:main with commit 4b54083 Oct 2, 2025
4 checks passed
@ankitathomas ankitathomas deleted the readme branch October 2, 2025 20:09
Copy link
Member

@rashmigottipati rashmigottipati left a comment

Choose a reason for hiding this comment

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

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants