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 opm docs #149

Merged
merged 2 commits into from
Jan 24, 2020
Merged

Conversation

kevinrizza
Copy link
Member

  • Add documentation that defines the input and output of all public opm
    commands

Description of the change:

Motivation for the change:

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /docs
  • Commit messages sensible and descriptive

Sorry, something went wrong.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 14, 2020
@kevinrizza
Copy link
Member Author

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 15, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@kevinrizza
Copy link
Member Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2020
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

Everything looks good. A few nits and questions.

README.md Outdated
│   └── servicemonitor.crd.yaml
└── prometheus.package.yaml

The operator-sdk also provides tooling to generate the metadata, Dockerfile, and even build the container image itself. Some basic documentation on how this works is located in [this repository's documentation](docs/design/operator-bundle.md#Operator-SDK-CLI).
Copy link
Member

Choose a reason for hiding this comment

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

Note here that "operator-sdk" reference is being removed on the bundle content validation PR. So we may need to update this later after the PR merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, that sounds good. I think we can just remove this and then update it once the sdk pushes their own doc we can link to.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@kevinrizza
Copy link
Member Author

@dinhxuanvu updated based on your feedback.


# Manifest format


We refer to a directory of files with one ClusterServiceVersion as a "bundle". A bundle is typically just a ClusterServiceVersion and the CRDs that define the owned APIs of the CSV, though additional objects may be included.
We refer to a directory of files with one ClusterServiceVersion as a "bundle". A bundle typically includes a ClusterServiceVersion and the CRDs that define the owned APIs of the CSV in its manifest directory, though additional objects may be included. It also includes an annotations file in its metadata folder which defines some higher level aggregate data that helps to describe the format and package information about how the bundle should be added into an index of bundles.
Copy link
Contributor

Choose a reason for hiding this comment

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

This says "typically includes", but if the invariants below are accurate, then this is what a bundle minimally includes, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

CRDs are not mandatory, actually. It minimally includes just the CSV.


# Bundle images

Using [OCI spec](https://github.com/opencontainers/image-spec/blob/master/spec.md) container images as a method of storing the manifest and metadata contents of individual bundles, `opm` interacts directly with these images to generate and incrementally update the database. Once you have your [manifests defined](https://operator-framework.github.io/olm-book/docs/packaging-an-operator.html#writing-your-operator-manifests) and have created a directory in the format defined above, building the image is as simple as defining a [Dockerfile](docs/design/operator-bundle.md#Bundle-Dockerfile) and building that image:
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 unclear on how flexible the directory structure is intended to be. Is a distinction made between a bundle directory structure described above and the layout of files in a bundle image?

I see the statement "bundle directories are identified solely by the fact that they contain a ClusterServiceVersion, which provides an amount of freedom for layout of manifests," and I'm not sure how that aligns with the proscribed metadata and manifests directories mentioned above.

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 directory we care about is the one on the image. You could, in theory, write your own Dockerfile that pulls files from lots of places to generate the directory structure in place. The flexibility is that the manifest directory can have certain other kube manifests optionally (currently that list includes rbac files and some prometheus stuff aside from the CSVs and CRDs). In future iterations this list can (and will be) extended. But all of that does need to live in the manifests/ subdirectory on your image.

Right now metadata/ has just the annotations.yaml file, but we are intending for this to be extended in the same fashion.

@dinhxuanvu
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2020
@dinhxuanvu
Copy link
Member

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, kevinrizza

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2020
@dinhxuanvu
Copy link
Member

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@exdx
Copy link
Member

exdx commented Jan 24, 2020

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit cb8d55a into operator-framework:master Jan 24, 2020
@ecordell
Copy link
Member

/retest

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

None yet

8 participants