📖 Update ClusterExtensionRevision API docs #2350
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @perdasilva Can we fix the emoj for 📖 ( docs )? |
perdasilva
changed the title
perdasilva
changed the title
There was a problem hiding this comment.
Pull Request Overview
This pull request updates the ClusterExtensionRevision API documentation to provide more comprehensive and detailed descriptions for developers and users.
- Enhanced top-level API documentation explaining the purpose and lifecycle of ClusterExtensionRevisions
- Improved field documentation with detailed explanations of behavior for spec and status fields
- Expanded condition documentation explaining all possible condition types, reasons, and their meanings
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| api/v1/clusterextensionrevision_types.go | Updated Go API type definitions with comprehensive docstrings for ClusterExtensionRevision types, fields, and constants |
| manifests/experimental.yaml | Generated CRD manifest with updated OpenAPI schema descriptions matching the Go API documentation |
| manifests/experimental-e2e.yaml | Generated E2E CRD manifest with updated OpenAPI schema descriptions matching the Go API documentation |
| helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml | Helm chart CRD with updated OpenAPI schema descriptions matching the Go API documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml
Outdated
Show resolved
Hide resolved
|
@perdasilva I think it is also valid to address the Copilot reviews. WDYT? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2350 +/- ##
==========================================
+ Coverage 74.39% 74.42% +0.02%
==========================================
Files 93 93
Lines 7300 7300
==========================================
+ Hits 5431 5433 +2
+ Misses 1435 1434 -1
+ Partials 434 433 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Done =D |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, rashmigottipati, tmshort 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 |
openshift-ci
bot
added
the
approved
good thinking! thank you!! |
michaelryanpeter
left a comment
michaelryanpeter
left a comment
There was a problem hiding this comment.
So I've looked it over, and I think it is pretty good overall, but I find the way that the LLM described the fields to be really confusing. For example, "phases is an optional, immutable list of phases..." or "objects is...". I think that something like "the phases property/field specifies an optional, immutaable list of phases. A phase is a ..." would be easier to grok. But I don't think this is a blocker and I know time is tight. Since it is tech preview, we can update these comments right?
| Objects within a phase are applied simultaneously, and all objects must pass their | ||
| readiness probes before the revision progresses to the next phase. Phases are applied | ||
| in a well-defined order based on the types of objects they contain. |
There was a problem hiding this comment.
Doesn't this contradict line 116?
|
New changes are detected. LGTM label has been removed. |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
Signed-off-by: Per Goncalves da Silva <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // | ||
| // +kubebuilder:default="Active" | ||
| // +kubebuilder:validation:Enum=Active;Paused;Archived | ||
| // +kubebuilder:validation:XValidation:rule="oldSelf == 'Active' || oldSelf == 'Paused' || oldSelf == 'Archived' && oldSelf == self", message="can not un-archive" |
There was a problem hiding this comment.
The validation error message should use "cannot" instead of "can not". "Cannot" is the standard single-word form preferred in error messages.
| // +kubebuilder:validation:XValidation:rule="oldSelf == 'Active' || oldSelf == 'Paused' || oldSelf == 'Archived' && oldSelf == self", message="can not un-archive" | |
| // +kubebuilder:validation:XValidation:rule="oldSelf == 'Active' || oldSelf == 'Paused' || oldSelf == 'Archived' && oldSelf == self", message="cannot un-archive" |
5 participants
Description
Update the ClusterExtensionRevision API docstrings to provide more thorough API documentation for the user.
Note: changes were generated using AI tools and manually updated
Reviewer Checklist