Skip to content
This repository has been archived by the owner on Aug 12, 2024. It is now read-only.

(docs): Update plain bundle docs #642

Merged

Conversation

everettraven
Copy link
Contributor

@everettraven everettraven commented Jun 21, 2023

Description

  • Removes docs/concepts/plain-bundle-spec.md in favor of moving its contents to docs/bundles/plain.md (IMO a more intuitive location to find information on plain bundles)
  • Replaces the Quickstart section from the old docs/concepts/plain-bundle-spec.md with a section title Building a plain bundle that includes information about how to build a plain bundle for use with all the various source types that rukpak supports

Motivation

  • Improve the documentation on how to build a plain bundle for use with rukpak

@everettraven everettraven requested a review from a team as a code owner June 21, 2023 14:23
docs/bundles/plain.md Outdated Show resolved Hide resolved
Copy link
Member

@stevekuznetsov stevekuznetsov left a comment

Choose a reason for hiding this comment

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

A lot of these comments are just suggestions on how the terminology/formatting/etc can be more consistent, which can be a real boon for new readers.

docs/bundles/plain.md Outdated Show resolved Hide resolved
docs/bundles/plain.md Outdated Show resolved Hide resolved
docs/bundles/plain.md Outdated Show resolved Hide resolved
docs/bundles/plain.md Outdated Show resolved Hide resolved
docs/bundles/plain.md Outdated Show resolved Hide resolved
docs/bundles/plain.md Show resolved Hide resolved
docs/bundles/plain.md Show resolved Hide resolved
docs/bundles/plain.md Show resolved Hide resolved
docs/bundles/plain.md Show resolved Hide resolved
docs/bundles/plain.md Show resolved Hide resolved
Copy link
Member

@stevekuznetsov stevekuznetsov left a comment

Choose a reason for hiding this comment

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

Looks good!

@stevekuznetsov
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2023
* An [upload](../sources/upload.md)
* A `.tgz` file returned by a [http endpoint](../sources/http.md)

Additional source types (such as a local volume or a generic URI-based resource) are on the roadmap. These source types all present the same content: a directory containing static Kubernetes YAML manifests
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 drop this line. There's nothing on the immediate roadmap at this point AFAIK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 56cfbae

@@ -29,16 +58,123 @@ manifests
└── deployment.yaml
```

For a bundle `git` repo, any directory that contains only static Kubernetes manifests checked into a `git` repository
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is quite true. A plain bundle must have a manifests directory (yes, specifically named manifests) at its root. So it can't be any directory. The bundle root directory has to contain a manifests directory.

Copy link
Member

Choose a reason for hiding this comment

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

TBH, I don't think I'd talk about the source types here though. The bundle format and the sourcing mechanism are completely decoupled in the rukpak design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it in 56cfbae

Comment on lines 39 to 78
* The static manifests must be located in the root-level `/manifests` directory in a bundle image for the bundle to be a
valid `plain+v0` bundle that the provisioner can unpack. A plain bundle image without a `/manifests` directory is
invalid and will not be successfully unpacked onto the cluster.
* For a bundle `git` repo, the manifests directory can be anywhere in the repository, not just at the root-level. The
location can be specified via `spec.source.git.directory`. There must be a flat directory consisting of Kubernetes YAML manifests at the provided location in order to have a valid bundle git repo. If a specific directory is not provided, it is assumed to be `./manifests` in a bundle git repo.
* The manifests directory should be flat: all manifests should be at the top-level with no subdirectories.
* Including any content in the /manifests directory of a plain bundle that is not static manifests will result in
a failure when creating content on-cluster from that bundle. Essentially, any file that would not
successfully `kubectl apply` will result in an error, but multi-object YAML files, or JSON files, are fine. There will
be validation tooling provided that can determine whether a given artifact is a valid bundle. No newline at end of file
* The plain bundle image can be built from any base image, but `scratch` is recommended as it keeps the resulting bundle
image a minimal size.
* It is required that `kubectl apply` is able to process all .yaml files in the directory that make up a plain bundle. For example, multi-object YAML files are acceptable, but Ansible playbooks would not be.
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 try to rewrite these to be focused on the contents of a plain bundle root directory, and not at all aware of the source implementation. Then the source docs should talk about the specifics of handing a bundle root off to a provisioner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if 56cfbae suffices

docs/bundles/plain.md Outdated Show resolved Hide resolved
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

I left a few comments in the spec section of the doc suggesting that we remove references to the sources.

However, I think the Building a plain bundle section, with its source sub-sections is good and should remain here.

@openshift-ci
Copy link

openshift-ci bot commented Jun 28, 2023

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2023
@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@079b611). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #642   +/-   ##
=======================================
  Coverage        ?   31.94%           
=======================================
  Files           ?       10           
  Lines           ?      479           
  Branches        ?        0           
=======================================
  Hits            ?      153           
  Misses          ?      304           
  Partials        ?       22           

docs/bundles/plain.md Show resolved Hide resolved
docs/bundles/plain.md Outdated Show resolved Hide resolved
docs/bundles/plain.md Outdated Show resolved Hide resolved
docs/bundles/plain.md Show resolved Hide resolved
docs/bundles/plain.md Show resolved Hide resolved
docs/bundles/plain.md Show resolved Hide resolved
docs/bundles/plain.md Outdated Show resolved Hide resolved
docs/bundles/plain.md Outdated Show resolved Hide resolved
docs/bundles/plain.md Outdated Show resolved Hide resolved
docs/bundles/plain.md Outdated Show resolved Hide resolved
@everettraven everettraven requested a review from fgiloux July 6, 2023 19:59
@everettraven everettraven merged commit 8f55f84 into operator-framework:main Jul 10, 2023
11 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants