Skip to content

Conversation

azych
Copy link
Contributor

@azych azych commented Mar 13, 2025

'Extension(s)' is the target nomenclature for what olmv1 is/will be able to handle and is used throughout our official documentation.
This PR changes all 'operator(s)' references in olmv1 commands and code to reflect that.

@openshift-ci openshift-ci bot requested review from benluddy and jmrodri March 13, 2025 11:01
@azych azych force-pushed the olmv1-operator-to-extension branch from b1ee82b to 88bbd57 Compare March 13, 2025 11:02
@azych
Copy link
Contributor Author

azych commented Mar 13, 2025

/hold

let's get this in after #226

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 13, 2025
@azych azych force-pushed the olmv1-operator-to-extension branch from 88bbd57 to 66b6f5d Compare March 13, 2025 15:10
@azych
Copy link
Contributor Author

azych commented Mar 13, 2025

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 13, 2025
Comment on lines +16 to +17
Use: "install <extension>",
Short: "Install an extension",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Use: "install <extension>",
Short: "Install an extension",
Use: "install <package name>",
Short: "Install a cluster extension for a package",

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'm not sure about replacing extension with package name here. I think commonly and for user-facing purposes you'd say you want to install an (cluster)extension?
Package name seems like an implementation detail at this point and at least to me it could confuse people because we'd have crud operations mentioning (cluster)extension and then a package terminology at the root level.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Ankita. Here is my suggestion

Use:   "install <cluster extension name>",
		Short: "Install a cluster extension",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a simple rename PR and @LalatenduMohanty is working on install/uninstall commands that this comment touches. How about we have those discussions on that incoming PR where they belong?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with redoing the install help text in the v1 install/uninstall commands PR.

Copy link
Member

Choose a reason for hiding this comment

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

Thats fine. I will fix it in my PR.

Comment on lines +16 to +17
Use: "install <extension>",
Short: "Install an extension",
Copy link
Member

Choose a reason for hiding this comment

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

I agree with Ankita. Here is my suggestion

Use:   "install <cluster extension name>",
		Short: "Install a cluster extension",

Warning: this command permanently deletes objects from the cluster. If the
uninstalled Operator bundle contains CRDs, the CRDs will be deleted, which
uninstalled extension bundle contains CRDs, the CRDs will be deleted, which
Copy link
Member

Choose a reason for hiding this comment

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

we should not use extension and bundle at the same time. Use of bundle is associated with registry v1 content but not much with cluster extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no functional change, it's just one-word rename, so I think bundle(s) are still being used to install operators/extensions.

That said, this is a simple rename PR and @LalatenduMohanty is working on install/uninstall commands that this comment touches. How about we have those discussions on that incoming PR where they belong?

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 17, 2025
@LalatenduMohanty LalatenduMohanty added this pull request to the merge queue Mar 18, 2025
Merged via the queue into operator-framework:main with commit b496b10 Mar 18, 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