Skip to content
This repository was archived by the owner on Aug 2, 2021. It is now read-only.

refine install and dependency docs #21

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dmesser
Copy link
Contributor

@dmesser dmesser commented Dec 2, 2019

Signed-off-by: dmesser [email protected]

@dmesser dmesser requested a review from awgreene December 2, 2019 13:30
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 2, 2019
Signed-off-by: dmesser <[email protected]>
Copy link
Member

@Bowenislandsong Bowenislandsong left a comment

Choose a reason for hiding this comment

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

Great PR, Thank you for cleaning some of the stuff up. Just some nits.


```yaml
k describe subs subs-to-my-operator -n olm
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we want to say $ kubectl instead of k?


If the required API cannot be resolved, OLM will not install operators that rely on that API.
If are required API cannot be resolved or found, OLM will not install operators that rely on that API.
Copy link
Member

Choose a reason for hiding this comment

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

nit: I don't understand why we changed the to are...

Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

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

Nice work @dmesser, a few comments but the suggested changes look great.

# How do I install my operator with OLM?
# How do I install my operator with OLM?

[Once you've made your packaged operator available in a catalog](packaging-an-operator.md) it will appear in the `packagemanifest` list from which the Operators to install are selected. See [How do I list available Operators](list-available-operators.md#information-relevant-for-installation) how to retrieve the required information from the `PackageServer` in order start an installation of an Operator.
Copy link
Member

Choose a reason for hiding this comment

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

The following sentence does not read well:

See [How do I list available Operators] how to retrieve...

$ kubectl describe packagemanifest my-operator
```

With this information a `Subscription` object can be created. This represents the intent to install an Operator from a particular catalog and keep it updated throughout the life cycle of the Operator via newly released version that got subsequently added to the referenced catalog.
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend changing

This represents the intent to install an Operator from a particular catalog and keep it updated throughout the life cycle of the Operator via newly released version that got subsequently added to the referenced catalog.

to

A subscription represents the desire to install an Operator from a particular catalog and keep it updated as new versions are subsequently added to the referenced catalog.

or somethings along those lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe even

With this information, a Subscription to an operator can be created.

instead of

With this information a Subscription object can be created.

And in @awgreene 's suggestion:

A subscription represents the desire to install an Operator from a particular catalog and keep it updated as new versions are subsequently added to the referenced catalog.

if we tweak it to

A subscription represents the desire to install an Operator from a particular catalog, and subscribe to updated version of the operators as they are subsequently added to the catalog.

then if could read like more of a user facing doc than dev facing doc.

For example, if you want to install an operator named `my-operator`, from a catalog named `my-catalog` that is in the namespace `olm`, and you want to subscribe to the channel `stable`, your subscription yaml would look like this:
```

The `spec.channel` property can also be omitted in which case the default channel will be picked. Optionally you can also define `spec.startingCSV` to denote that you want to install a specific version of your Operator and not the latest.
Copy link
Member

Choose a reason for hiding this comment

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

Should we highlight behavior when specifying spec.startingCSV with automatic approval? My concern is that users will set a startingCSV expecting that the specified version will persist on cluster and be surprised when it is updated.

NAME READY UP-TO-DATE AVAILABLE AGE
my-operator 1/1 1 1 9m48s
```
Copy link
Member

Choose a reason for hiding this comment

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

Should we highlight that a new installPlan for the next available version will be created when available? This will give users an idea how to perform a manual update.

@@ -1,17 +1,24 @@
# How do I list Operators available to install?

There is an extension API in OLM named `PackageManifest` that contains information about existing `CatalogSources`, which is essentially a repository of CSVs, CRDs, and packages that define an operator in the cluster. By querying that API, you can see the list of available operators.
There is an extension API in OLM named `PackageManifest` that contains information about the content served by `CatalogSources`, which is essentially a repository of CSVs, CRDs, and packages that define a operator in the cluster. By querying that API, you can see the list of available operators.
Copy link
Member

Choose a reason for hiding this comment

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

a operator

This should be:

an operator

Comment on lines +32 to +41
If there are no `CatalogSource` objects present in the current namespace of the command above the list will contain all Operators found in global catalogs. To include Operators available to install from namespace catalogs simply supply it with the `kubectl` command:

```bash
$ kubectl get packagemanifests

NAME CATALOG AGE
...
my-operator My Catalog 31m
...
```
Copy link
Member

Choose a reason for hiding this comment

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

As someone that has worked with OLM before this is clear, but it may not be clear to users unfamiliar with OLM.

NAME CATALOG AGE
...
my-operator My Catalog 31m
...
Copy link
Member

Choose a reason for hiding this comment

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

You used [...] above to imply that additional output existed. Please chose a single format.

@@ -147,7 +147,7 @@ spec:

Any operator tied to this `OperatorGroup` will now be confined to the permission(s) granted to the specified `ServiceAccount`. If the operator asks for permission(s) that are outside the scope of the `ServiceAccount` the install will fail with appropriate error(s).

An example of scoping an operator can be found [here]("https://operator-framework.github.io/olm-book/docs/how-do-i-scope-down-an-operator).
An example of scoping an operator can be found [here](./).
Copy link
Member

Choose a reason for hiding this comment

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

We can probably remove this.

Copy link
Contributor

@anik120 anik120 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @dmesser. Had a few comments.


```bash
kubectl patch installplan install-q4fmf -n olm --type merge -p '{"spec":{"approved":true}}'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we get rid of the "What do I approve an update?" doc too if we're putting this information here?


## Install with automatic updates enabled

First, retrieve the package name, channel and catalog name and catalog source namespace from the `packagemanifest` of the desired Operator to install.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: package name, channel, CatalogSource name, and CatalogSource namespace from the packagemanifest...

@@ -6,7 +6,7 @@ Checking the presence of custom installed operators in the cluster is simply a m
associated with it that that contains all the details of the operator. Running `kubectl get csvs -A`
would return all CSVs across all namespaces and provide a high-level view of all custom installed operators.

If the ClusterServiceVersion fails to show up or does not reach the `Succeeded` phase, please check the [troubleshooting documentation](https://) to debug your installation.
If the ClusterServiceVersion fails to show up or does not reach the `Succeeded` phase, please check the [troubleshooting documentation](troubleshooting.md) to debug your installation.
Copy link
Contributor

Choose a reason for hiding this comment

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

We could link to (troubleshooting.md#clusterserviceversion-troubleshooting) to link to the more relevant section instead of the entire doc.

NAME READY UP-TO-DATE AVAILABLE AGE
<name-of-your-operator> 1/1 1 1 9m48s

If the ClusterServiceVersion fails to show up or does not reach the `Succeeded` phase, please check the [troubleshooting documentation](troubleshooting.md) to debug your installation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above about the (troubleshooting.md#clusterserviceversion-troubleshooting).

$ kubectl describe packagemanifest my-operator
```

With this information a `Subscription` object can be created. This represents the intent to install an Operator from a particular catalog and keep it updated throughout the life cycle of the Operator via newly released version that got subsequently added to the referenced catalog.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe even

With this information, a Subscription to an operator can be created.

instead of

With this information a Subscription object can be created.

And in @awgreene 's suggestion:

A subscription represents the desire to install an Operator from a particular catalog and keep it updated as new versions are subsequently added to the referenced catalog.

if we tweak it to

A subscription represents the desire to install an Operator from a particular catalog, and subscribe to updated version of the operators as they are subsequently added to the catalog.

then if could read like more of a user facing doc than dev facing doc.

```
$ kubectl get csv -n <namespace-operator-was-installed-in>

The `Subscription` has successfully deployed the Operator when the `status.state` property reaches `AtLatestKnown`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this always true? If the CSV reaches a Failed state, will the Subscription not reach AtLatestKnow?

@openshift-ci-robot
Copy link

@dmesser: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants