chore(docs): add RFC describing OCM package capabilities#60
Open
trevex wants to merge 1 commit into
Open
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 33 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds RFC 01: a design for a typed OCM "capabilities" resource (requires/provides), strict YAML schema, hybrid verification via cluster/catalog probes, a proposed ocm-kit/capabilities Go API (Probe interface and orchestration), CLI subcommands, testing/rollout guidance, and v2 deferred items. ChangesOCM Package Capabilities RFC
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
docs/rfcs/rfc01-ocm-package-capabilities.md (4)
355-358: ⚡ Quick winConsider documenting type assertion contract for
Requirement.Detail.The
Requirementstruct usesDetail any, which will require type assertions in implementing code:switch req.Kind { case KubernetesVersion: k := req.Detail.(KubernetesRequirement) // ... }This pattern is common in Go but error-prone. Consider:
- Adding a code comment documenting the
RequirementKind → Detailtype mapping, or- Providing a helper method like
req.AsKubernetes() (*KubernetesRequirement, bool)for safer access, or- Evaluating whether a sealed interface (types implementing a private marker method) would provide better type safety
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/rfcs/rfc01-ocm-package-capabilities.md` around lines 355 - 358, The Requirement struct's Detail field is typed as any which forces callers to use unchecked type assertions; add one of the following to make the contract explicit and safer: (a) document the mapping between RequirementKind and the concrete Detail type in a comment above type Requirement (e.g., RequirementKind → KubernetesRequirement, ClusterRequirement, etc.), or (b) add safe helper accessors such as AsKubernetes() (*KubernetesRequirement, bool) and AsCluster() (*ClusterRequirement, bool) on Requirement to perform the type assertion and return a boolean for success, or (c) replace Detail with a sealed interface and concrete types that implement a private marker method to enforce the allowed variants; update code that switches on RequirementKind (and any callers of Requirement.Detail) to use the new helpers or interface.
402-402: 💤 Low valueDocument rationale for exit code 64.
Exit code
64follows the BSDsysexits.hconvention for usage errors (EX_USAGE), which is a good choice for CLI tools. Consider adding a brief note to clarify this isn't arbitrary:-| **Usage error** | Caller passed bad input | `64` | (returned from `Verify`) | +| **Usage error** | Caller passed bad input | `64` (EX_USAGE) | (returned from `Verify`) |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/rfcs/rfc01-ocm-package-capabilities.md` at line 402, Add a short rationale below the table clarifying that exit code 64 is chosen to follow the BSD sysexits.h convention (EX_USAGE) for usage errors; mention that this maps to callers passing bad input and that the `Verify` function returns this code for CLI usage errors to make the choice explicit rather than arbitrary.
53-77: 💤 Low valueAdd language specifiers to fenced code blocks.
Three fenced code blocks are missing language specifiers, which markdownlint flags. Adding them improves rendering and accessibility:
- Line 53: ASCII architecture diagram → add
text- Line 275: Directory structure → add
text- Line 435: CLI output example → add
text📝 Suggested fixes
-``` +```text +-------------------------------+-``` +```text ocm-kit/ capabilities/-``` +```text $ ocm-kit capabilities verify ./arc:0.2.0 \Also applies to: 275-284, 435-449
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/rfcs/rfc01-ocm-package-capabilities.md` around lines 53 - 77, Add the missing Markdown fenced code block language specifiers by changing the three plain triple-backtick blocks to use "text": the ASCII architecture diagram block (the block starting with " +-------------------------------+" / "OCM ComponentVersion"), the directory structure block that begins with "ocm-kit/" (the tree showing capabilities/), and the CLI output example block that starts with "$ ocm-kit capabilities verify ..."; update each opening ``` to ```text so markdownlint stops flagging them and rendering improves.
96-96: ⚡ Quick winStandardize semver constraint format across examples.
The semver version constraints use inconsistent formatting:
- Line 96:
">=1.27.0, <1.31.0"(space after comma, patch versions)- Line 190 & 216:
">=1.25"(no patch version)- Line 246:
">=1.27,<1.31"(no space after comma, no patch version)While
github.com/Masterminds/semver/v3may accept all these formats, consistent formatting across the RFC improves clarity for implementers.📝 Suggested standardization
Pick one format (e.g.,
">=1.27.0, <1.31.0"with spaces and patch versions) and apply it consistently:- version: ">=1.27,<1.31" + version: ">=1.27.0, <1.31.0"- version: ">=1.25" + version: ">=1.25.0"- version: ">=1.28" + version: ">=1.28.0"Also applies to: 190-190, 216-216, 246-246
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/rfcs/rfc01-ocm-package-capabilities.md` at line 96, Normalize all semver constraint examples to a single consistent format (use full patch versions and a space after the comma), e.g. convert occurrences like ">=1.25", ">=1.27,<1.31" and ">=1.27.0, <1.31.0" to the standardized form ">=1.27.0, <1.31.0" throughout the RFC (update the literal strings found in the document where version constraints appear around the examples at the noted locations); ensure every semver constraint uses patch versions and a single space after commas for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/rfcs/rfc01-ocm-package-capabilities.md`:
- Line 267: The narrative claims an "unverified" state for requirements but the
Result struct (Result with fields Satisfied, Missing, Errors) and probe
semantics (probes returning satisfied=false with no errors => Missing) have no
such state; reconcile by either updating the API or the doc: either add an
Unverified []Requirement field to Result and ensure probe logic populates it
when no probe ran/applicable, or change the narrative at line 267 to state the
requirement will be reported as Missing (with metadata explaining no applicable
probe), or adopt the Errors approach by defining and using a specific error type
(e.g., ErrNoApplicableProbe) so such cases appear in Errors; pick one approach
and update the Result definition, probe handling code (probe evaluation path),
and the RFC narrative to match consistently.
---
Nitpick comments:
In `@docs/rfcs/rfc01-ocm-package-capabilities.md`:
- Around line 355-358: The Requirement struct's Detail field is typed as any
which forces callers to use unchecked type assertions; add one of the following
to make the contract explicit and safer: (a) document the mapping between
RequirementKind and the concrete Detail type in a comment above type Requirement
(e.g., RequirementKind → KubernetesRequirement, ClusterRequirement, etc.), or
(b) add safe helper accessors such as AsKubernetes() (*KubernetesRequirement,
bool) and AsCluster() (*ClusterRequirement, bool) on Requirement to perform the
type assertion and return a boolean for success, or (c) replace Detail with a
sealed interface and concrete types that implement a private marker method to
enforce the allowed variants; update code that switches on RequirementKind (and
any callers of Requirement.Detail) to use the new helpers or interface.
- Line 402: Add a short rationale below the table clarifying that exit code 64
is chosen to follow the BSD sysexits.h convention (EX_USAGE) for usage errors;
mention that this maps to callers passing bad input and that the `Verify`
function returns this code for CLI usage errors to make the choice explicit
rather than arbitrary.
- Around line 53-77: Add the missing Markdown fenced code block language
specifiers by changing the three plain triple-backtick blocks to use "text": the
ASCII architecture diagram block (the block starting with "
+-------------------------------+" / "OCM ComponentVersion"), the directory
structure block that begins with "ocm-kit/" (the tree showing capabilities/),
and the CLI output example block that starts with "$ ocm-kit capabilities verify
..."; update each opening ``` to ```text so markdownlint stops flagging them and
rendering improves.
- Line 96: Normalize all semver constraint examples to a single consistent
format (use full patch versions and a space after the comma), e.g. convert
occurrences like ">=1.25", ">=1.27,<1.31" and ">=1.27.0, <1.31.0" to the
standardized form ">=1.27.0, <1.31.0" throughout the RFC (update the literal
strings found in the document where version constraints appear around the
examples at the noted locations); ensure every semver constraint uses patch
versions and a single space after commas for consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 847715a2-0dfd-49dc-8efe-dd9b86316708
📒 Files selected for processing (1)
docs/rfcs/rfc01-ocm-package-capabilities.md
trevex
force-pushed
the
chore/rfc-ocm-package-capabilities
branch
from
d37fb7b to
a8ef490
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/rfcs/rfc01-ocm-package-capabilities.md`:
- Around line 53-77: Several fenced code blocks in the RFC (the ASCII diagram
beginning with "+-------------------------------+", the tree output starting
with "ocm-kit/ capabilities/", and the CLI transcript starting with "$ ocm-kit
capabilities verify") are missing language identifiers and causing markdownlint
MD040 failures; update those fences to include explicit languages (use "text"
for the ASCII diagram and the tree output, and "bash" for the CLI transcript) so
each ```... block becomes ```text or ```bash accordingly, and do the same for
the other two occurrences noted (lines around 275-284 and 449-463) to satisfy
linting.
- Around line 407-415: The RFC must define a deterministic precedence rule for
mixed outcome runs: add a clear ordering (e.g. Usage error (64) highest, then
Probe error (2), then Unverified (3), then Unsatisfied/Missing (1)) and state
that the CLI flags --fail-on-probe-error and --fail-on-unverified promote those
classes to be treated as failures and thus affect the chosen exit code; update
the textual spec and the behavior of Verify to compute the final exit code by
scanning results in that precedence order (and document identical wording for
the other section at lines ~447-463).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a3c04eb-48b3-480f-821f-07c59dec6cf9
📒 Files selected for processing (1)
docs/rfcs/rfc01-ocm-package-capabilities.md
Comment on lines
+53
to
+77
| ``` | ||
| +-------------------------------+ | ||
| | OCM ComponentVersion | | ||
| | | | ||
| | resources: | | ||
| | - helm-values-template | | ||
| | - capabilities.yaml <- new | | ||
| | - chart, images, ... | | ||
| +---------------+---------------+ | ||
| | | ||
| v (label-discovered) | ||
| +-----------------------------------------------------------+ | ||
| | ocm-kit / capabilities package | | ||
| | | | ||
| | GetCapabilities ----> typed Go struct (v1 schema) | | ||
| | Verify(opts) ----> Result {satisfied,missing,errors} | | ||
| | | | | ||
| | +-- ClusterProbe (kubeconfig: api-discovery, nodes) | | ||
| | +-- CatalogProbe (other components' provides) | | ||
| +-----------------------------------------------------------+ | ||
| | | | ||
| v v | ||
| ocm-kit CLI subcommands external installers | ||
| (show / verify / resolve) (consume library) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifiers to fenced code blocks to satisfy markdownlint.
Three fenced blocks are missing language tags (MD040), which can fail docs linting. Add explicit languages (e.g., text for diagrams/tree output, bash for shell transcripts).
Suggested patch
-```
+```text
+-------------------------------+
| OCM ComponentVersion |
@@
-```
+```text-```
+```text
ocm-kit/
capabilities/
@@
-```
+```text-```
+```bash
$ ocm-kit capabilities verify ./arc:0.2.0 \
--kubeconfig ~/.kube/config \
@@
2 of 5 unsatisfied. Exit 1.
-```
+```Also applies to: 275-284, 449-463
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/rfcs/rfc01-ocm-package-capabilities.md` around lines 53 - 77, Several
fenced code blocks in the RFC (the ASCII diagram beginning with
"+-------------------------------+", the tree output starting with "ocm-kit/
capabilities/", and the CLI transcript starting with "$ ocm-kit capabilities
verify") are missing language identifiers and causing markdownlint MD040
failures; update those fences to include explicit languages (use "text" for the
ASCII diagram and the tree output, and "bash" for the CLI transcript) so each
```... block becomes ```text or ```bash accordingly, and do the same for the
other two occurrences noted (lines around 275-284 and 449-463) to satisfy
linting.
trevex
force-pushed
the
chore/rfc-ocm-package-capabilities
branch
from
a8ef490 to
a8be4cb
Compare
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Currently we have no way to validate the requirements/dependencies/prerequisites of OCM packages. This RFC aims to solve this problem.
Closes #
Why
We want to be able to aid users better, validate` OCM deployments and potentially create DAGs for deployments.
Testing
Only RFC.
Notes for reviewers
Please have a read.
Checklist
Summary by CodeRabbit