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

Initial docs for file-based catalogs #177

Merged
merged 13 commits into from
Sep 10, 2021

Conversation

joelanford
Copy link
Member

Signed-off-by: Joe Lanford [email protected]

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2021
@openshift-ci openshift-ci bot requested review from dinhxuanvu and hasbro17 July 29, 2021 14:11
@joelanford joelanford changed the title wip: initial docs for declarative config Initial docs for declarative config Aug 12, 2021
@joelanford joelanford removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 12, 2021
@joelanford joelanford marked this pull request as ready for review August 12, 2021 15:07
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.

This looks great!

I have a few minor comments, nothing blocking

content/en/docs/Reference/declarative-config.md Outdated Show resolved Hide resolved
content/en/docs/Reference/declarative-config.md Outdated Show resolved Hide resolved
content/en/docs/Reference/declarative-config.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

@joelanford thank you for really well written doc!

I'm trying to think how we can plug this into our existing doc site structure however. In https://olm.operatorframework.io/docs/tasks/creating-an-index/ , we're still talking about using imperative opm commands to build a sqllite index. We should probably replace that with opm commands to create a file based catalogs.

We can decided to keep that doc limited to whatever's required to get an index up and running, and then in the end link to this doc saying "For more information, check the reference"

Also, in this doc we talk about advantages of file based catalogs over sqllite catalogs. As a brand new user who's coming to the site for the first time, is it necessary for me to know the history about sqllite based indexes? I think some of the write up about file based catalogs initially are really cool for an enhancement that's proposing to replace the existing work of sqllite based indexes. As a user who's coming here to know how to create an index though, I'd probably be more comfortable using my attention span to read whatever I need about creating an index (file based or otherwise, as long as it's just one of them).

Essentially, we might be able to split this doc into two, one being "tasks/creating-an-index" (i.e replacing existing content), and the second being this doc, which contains "further details" about opm commands/workflows/best practices etc. This doc is also where I'm thinking we could link am example index github repo that follows gitops practices to accept PRs and create images etc.


### Schema

At its core, file-based catalogs use a simple format that can be extended with arbitrary schemas. The format that all file-based catalog blobs must adhere to is the `Meta` schema. The below [cue][cuelang-spec] `_Meta` schema defines all file-based catalog blobs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we define these as go structs in the registry repo? Is there any distinct advantage of listing out these schemas as CUE definition rather than just copy pasting the structs we use?

Copy link
Member Author

Choose a reason for hiding this comment

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

CUE is more expressive in terms of what valid values are, but:

  1. As currently stated, the CUE specs don't encode all validations, so its not a full spec. (e.g. valid base64 data)
  2. CUE is somewhat esoteric still, and few people really know CUE syntax (though I think it is fairly straightforward to grok, at least for what I've written here)

I can make arguments in both directions. I think if I used the Go structs, we might have to add more narrative about what's valid/invalid. I think documenting "just use opm validate" isn't enough because that encodes the spec, but it isn't the spec.

@joelanford
Copy link
Member Author

joelanford commented Aug 23, 2021

@anik120 I totally anticipated your suggestions. I wanted to get a bunch of stuff written down/reviewed and then have the conversation about where to slot all of it into the existing site structure.

Based on your list I think we need to:

  • rewrite https://olm.operatorframework.io/docs/tasks/creating-an-index/ to focus on file-based configs
  • leave a section in that doc about imperative/sqlite commands, but clearly mark it as deprecated
  • split the opm cli (if it doesn't already exist) docs into a separate page, and ideally have that page be autogenerated by cobra and included in the sanity check during CI.
  • split the DC spec into a different doc (maybe under the "Reference" section?), and link to it from "Creating an Index"

Also, in this doc we talk about advantages of file based catalogs over sqllite catalogs. As a brand new user who's coming to the site for the first time, is it necessary for me to know the history about sqllite based indexes?

Yeah, I agree with this. Do you think it could make sense to leave it in under the deprecated sqlite docs, like a "here's all the good reasons you should invest in migrating ASAP" kind of thing? That way:

  1. Folks using and refering to sqlite/imperative docs would see it.
  2. We'd be much more likely to remove it when sqlite/imperative command support is removed.

Copy link
Contributor

@timflannagan timflannagan 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 tackling this - I thought this was really well written and provided a good overview and deep dive into the CLI and new structuring of spec/metadata. Just had a couple of quick comments/suggestions/etc.

content/en/docs/Reference/file-based-catalogs.md Outdated Show resolved Hide resolved
content/en/docs/Reference/file-based-catalogs.md Outdated Show resolved Hide resolved
content/en/docs/Reference/file-based-catalogs.md Outdated Show resolved Hide resolved
content/en/docs/Reference/file-based-catalogs.md Outdated Show resolved Hide resolved
content/en/docs/Reference/file-based-catalogs.md Outdated Show resolved Hide resolved
content/en/docs/Tasks/creating-an-index.md Outdated Show resolved Hide resolved
content/en/docs/Tasks/creating-an-index.md Show resolved Hide resolved
@timflannagan
Copy link
Contributor

timflannagan commented Sep 2, 2021

I may be missing some context as well, but it wasn't immediately clear to me what level of familiarity of OLM we're assuming here. I think a new user discovering OLM, or an existing user that isn't up-to-date with the progressions (e.g. familiarity with sqlite-based indexes) would have some trouble getting up-to-speed on the new format without a quickstart-esq type documentation format. Is the intention to provide an overview and an exhaustive list of the new structure and CLI flags?

Edit: not suggesting we tackle this now, but something to think about further as we iterate on OLM v1's plan.

@joelanford joelanford changed the title Initial docs for declarative config Initial docs for file-based catalogs Sep 3, 2021
@timflannagan
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Sep 8, 2021
@timflannagan
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2021
Copy link
Collaborator

@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.

@joelanford this is coming along beautifully. The doc in Reference and the new "How to build a Catalog" doc are really, really well written, thanks a lot for taking the time to do that!

I've left some suggestions, and the overarching theme of my suggestions can probably be summarized with two points:

  1. We mention OLM specific details while describing a concept, when they aren't exactly necessary, a lot in our current docs. For example, is CSV the right word to use while talking about how to create upgrade graphs, or is a better, more suited replacement for that word is operator version?
  2. Somewhere along the line we lost the plot for index vs Catalogs, and we use these words interchangeably a lot. Makes sense for anyone who's been taught what an index is supposed to be, but it might possibly behoove us to try and recapture that plot a little and help our readers not be confused by just sticking to the use of the work Catalog through out this site. (I'll create an issue for replacing all occurrences of the word index with Catalog in docs not being touched by this PR so that we can tackle that separately)


## Methods for Specifying Updates

All update graphs are defined in file-based catalogs via `olm.channel` blobs. Each `olm.channel` defines the set of
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: are the words in file-based catalogs necessary? It makes sense to those of us who know about the old sqlite db based catalogs and the new file-based catalogs, but for someone new reading this, those words seems like might be adding additional cognitive load to process while reading this sentence.

Copy link
Collaborator

Choose a reason for hiding this comment

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

On second thought, what do you think about replacing those words with plaintext-backed storage and then linking to this doc https://deploy-preview-177--operator-lifecycle-manager.netlify.app/docs/reference/file-based-catalogs/

That doc could also be renamed to plaintext-backed storage catalogs. Just trying to think what will communicate the most about what these files are in the context of a catalog.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think @dmesser coined file-based catalog, which I thought was better than "declarative config", so I've been trying to use that term everywhere.

On this point, for now I'll add the link to the file-based catalog spec page as you suggested.

### Replaces

For explicit updates from one CSV to another, you can specify the CSV name to replace in your CSV as
For explicit updates from one CSV to another, you can specify the CSV name to replace in your channel entry as
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
For explicit updates from one CSV to another, you can specify the CSV name to replace in your channel entry as
For explicit updates from one operator version to another, you can specify the operator name to replace in your channel entry as

be included in your manifests or have already been added to that catalog (in the case where packaging
is done in the bundle format).
Note that it is not required for there to be an entry for `myoperator.v1.0.0` in the catalog as long as
other channel invariants (verified by `opm validate`) still hold. Generally, this means that the tail of the channel's
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will it be helpful to add a link to wherever opm validate is describe more fully here? I'm thinking about someone who just got sent this link to create an upgrade graph for olm, but hasn't interacted with the opm tool or DC before.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only doc that we could link would be the FBC spec doc that contains the CLI usage of opm validate. I'll add that link.

@@ -40,7 +45,7 @@ A subscription on `myoperator.v1.0.0` will update to `myoperator.v1.0.2` through

Installing from the UI today will always install the latest of a given channel. However, installing
specific versions is possible with this update graph by modifying the `startingCSV` field
of the subscription to point to the desired CSV name. Note that, in this case, the subscription will
of the subscription to point to the desired bundle name. Note that, in this case, the subscription will
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit

Suggested change
of the subscription to point to the desired bundle name. Note that, in this case, the subscription will
of the subscription to point to the desired operator bundle name. Note that, in this case, the subscription will

@@ -49,13 +54,16 @@ instead stays pinned to the specified version.
In order to skip through certain updates you can specify a list of CSV names to be skipped as such:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
In order to skip through certain updates you can specify a list of CSV names to be skipped as such:
In order to skip through certain updates you can specify a list of operator bundle names to be skipped as such:

Let's validate our index to see if we're ready to ship!
```sh
$ opm validate example-operator-index
FATA[0000] invalid index:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably should change this error message in opm too to say catalog instead of index.

└── channel must contain at least one bundle
```

Alright, so it's not valid. It looks like we need to add a bundle, so let's do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Alright, so it's not valid. It looks like we need to add a bundle, so let's do
Alright, so it's not valid. It looks like we need to add an operator bundle, so let's do

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a strong opinion here but to me this one seems unnecessary. A bundle is already a well-defined term for OLM and the thing we generate in the next section is an olm.bundle blob, not an olm.operator.bundle blob.


```sh
$ opm render quay.io/example-inc/example-operator-bundle:v0.1.0 \
--output=yaml >> example-operator-index/index.yaml
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
--output=yaml >> example-operator-index/index.yaml
--output=yaml >> example-catalog/catalog.yaml


Let's validate again:
```
$ opm validate example-operator-index
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$ opm validate example-operator-index
$ opm validate example-catalog

....I'll stop doing this now because I think I'm communicated my suggestion by now...


### Add a channel entry for the bundle

We rendered the bundle, but we still haven't yet added it to any channels.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
We rendered the bundle, but we still haven't yet added it to any channels.
We rendered the operator bundle, but we still haven't yet added it to any channels.

@joelanford
Copy link
Member Author

Somewhere along the line we lost the plot for index vs Catalogs, and we use these words interchangeably a lot. Makes sense for anyone who's been taught what an index is supposed to be, but it might possibly behoove us to try and recapture that plot a little and help our readers not be confused by just sticking to the use of the work Catalog through out this site. (I'll create an issue for replacing all occurrences of the word index with Catalog in docs not being touched by this PR so that we can tackle that separately)

I've been considering this point a bit recently as well. I've settled on "index" being the generic thing that contains one more packages, and "catalog" being a collection of indexes, which is effectively the same thing as a single index, except that there must be two or more packages.

Maybe that's an unhelpful distinction, but its where I've personally landed (and I'm not sure if that's reflected/consistent in this doc update, to be honest).

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2021
@anik120
Copy link
Collaborator

anik120 commented Sep 9, 2021

/approve.

Overall the docs look really great. We can follow up with the "cleaning up of communication" effort in a new PR.

@anik120
Copy link
Collaborator

anik120 commented Sep 9, 2021

/approve

@openshift-ci
Copy link

openshift-ci bot commented Sep 9, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anik120, joelanford

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2021
@timflannagan
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2021
@joelanford
Copy link
Member Author

/override lint

@openshift-ci
Copy link

openshift-ci bot commented Sep 10, 2021

@joelanford: /override requires a failed status context or a job name to operate on.
The following unknown contexts were given:

  • lint

Only the following contexts were expected:

  • Header rules - operator-lifecycle-manager
  • Mixed content - operator-lifecycle-manager
  • Pages changed - operator-lifecycle-manager
  • Redirect rules - operator-lifecycle-manager
  • netlify/operator-lifecycle-manager/deploy-preview
  • tide

In response to this:

/override lint

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.

@joelanford
Copy link
Member Author

/override test/lint

@openshift-ci
Copy link

openshift-ci bot commented Sep 10, 2021

@joelanford: /override requires a failed status context or a job name to operate on.
The following unknown contexts were given:

  • test/lint

Only the following contexts were expected:

  • Header rules - operator-lifecycle-manager
  • Mixed content - operator-lifecycle-manager
  • Pages changed - operator-lifecycle-manager
  • Redirect rules - operator-lifecycle-manager
  • netlify/operator-lifecycle-manager/deploy-preview
  • tide

In response to this:

/override test/lint

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants