add ADR for architectural decision that diverge from CAPI contracts #99
Conversation
WalkthroughThis pull request updates the project documentation by adding a new section titled "ADRs" in the Changes
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
openshift-ci
bot
added
approved
|
cc @jianzzha |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
README.md (1)
245-248: Improve Markdown List Styling
The newly added ADR section uses dashes (-) for unordered list items, but markdownlint expects asterisks (*) for consistency. Consider updating the list markers for uniformity. For example:- - [001-distribution-version](docs/adr/001-distribution-version.md) - - [002-distribution-version-upgrades](docs/adr/002-distribution-version-upgrades.md) + * [001-distribution-version](docs/adr/001-distribution-version.md) + * [002-distribution-version-upgrades](docs/adr/002-distribution-version-upgrades.md)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
246-246: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
247-247: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
docs/adr/001-distribution-version.md (1)
1-22: Clear and Comprehensive ADR Document
This ADR effectively explains the discrepancy between CAPI contracts and OpenShift versioning, and details the rationale for introducing thedistributionVersionfield. To further enhance documentation, consider a brief note on long-term strategies for upstream alignment.docs/adr/002-distribution-version-upgrades.md (2)
9-9: Language Improvement: Use "cannot" Instead of "can not"
For improved clarity and to adhere to common usage, please replace "can not" with "cannot". For example:- However, OpenShift versions can not be mapped directly to kubernetes versions. + However, OpenShift versions cannot be mapped directly to kubernetes versions.🧰 Tools
🪛 LanguageTool
[style] ~9-~9: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...es version. However, OpenShift versions can not be mapped directly to kubernetes versio...(CAN_NOT_PREMIUM)
29-31: Enhance Bullet Point Clarity with a Determiner
The bullet point currently reads:
- Diverges from the standard CAPI upgrade contract.Consider revising it to include a determiner for grammatical clarity. For example:
- Diverges from the standard CAPI upgrade contract. + It diverges from the standard CAPI upgrade contract.🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: A determiner appears to be missing. Consider inserting it.
Context: ...m the standard CAPI upgrade contract. - Current approach forcesdistributionVersionc...(AI_EN_LECTOR_MISSING_DETERMINER)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)docs/adr/001-distribution-version.md(1 hunks)docs/adr/002-distribution-version-upgrades.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
246-246: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
247-247: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🪛 LanguageTool
docs/adr/002-distribution-version-upgrades.md
[style] ~9-~9: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...es version. However, OpenShift versions can not be mapped directly to kubernetes versio...
(CAN_NOT_PREMIUM)
[uncategorized] ~31-~31: A determiner appears to be missing. Consider inserting it.
Context: ...m the standard CAPI upgrade contract. - Current approach forces distributionVersion c...
(AI_EN_LECTOR_MISSING_DETERMINER)
🔇 Additional comments (1)
docs/adr/002-distribution-version-upgrades.md (1)
1-34: Thorough ADR Document on Upgrade Management
This ADR provides a detailed explanation of the challenges in mapping OpenShift versions to Kubernetes versions and conveys the necessity for a centralized upgrade process. Overall, it’s well-structured and informative regarding both the current approach and future upstream discussions.🧰 Tools
🪛 LanguageTool
[style] ~9-~9: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...es version. However, OpenShift versions can not be mapped directly to kubernetes versio...(CAN_NOT_PREMIUM)
[uncategorized] ~31-~31: A determiner appears to be missing. Consider inserting it.
Context: ...m the standard CAPI upgrade contract. - Current approach forcesdistributionVersionc...(AI_EN_LECTOR_MISSING_DETERMINER)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (1)
246-247: Markdown List Style
The markdown linter (MD004) recommends using asterisks for unordered list items rather than dashes. Consider updating the list markers to asterisks for consistency with the expected style.- [001-distribution-version](docs/adr/001-distribution-version.md) - [002-distribution-version-upgrades](docs/adr/002-distribution-version-upgrades.md) + * [001-distribution-version](docs/adr/001-distribution-version.md) + * [002-distribution-version-upgrades](docs/adr/002-distribution-version-upgrades.md)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
246-246: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
247-247: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
docs/adr/002-distribution-version-upgrades.md (2)
9-9: Language Consistency
There is a minor language suggestion for improved readability. Replace "can not" with "cannot" to maintain consistency and common usage.- However, OpenShift versions can not be mapped directly to kubernetes versions. + However, OpenShift versions cannot be mapped directly to kubernetes versions.🧰 Tools
🪛 LanguageTool
[style] ~9-~9: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...es version. However, OpenShift versions can not be mapped directly to kubernetes versio...(CAN_NOT_PREMIUM)
31-31: Grammar Improvement
In this bullet point, consider inserting a determiner for clarity. For example, revising the sentence to start with "The risk of inconsistency..." can make the statement more complete.- Risk of inconsistency if upstream changes are not accepted or if future changes conflict with our approach. + The risk of inconsistency if upstream changes are not accepted or if future changes conflict with our approach.🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: A determiner appears to be missing. Consider inserting it.
Context: ...m the standard CAPI upgrade contract. - Current approach forcesdistributionVersionc...(AI_EN_LECTOR_MISSING_DETERMINER)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)docs/adr/001-distribution-version.md(1 hunks)docs/adr/002-distribution-version-upgrades.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/adr/001-distribution-version.md
🧰 Additional context used
🪛 LanguageTool
docs/adr/002-distribution-version-upgrades.md
[style] ~9-~9: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...es version. However, OpenShift versions can not be mapped directly to kubernetes versio...
(CAN_NOT_PREMIUM)
[uncategorized] ~31-~31: A determiner appears to be missing. Consider inserting it.
Context: ...m the standard CAPI upgrade contract. - Current approach forces distributionVersion c...
(AI_EN_LECTOR_MISSING_DETERMINER)
🪛 markdownlint-cli2 (0.17.2)
README.md
246-246: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
247-247: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🔇 Additional comments (3)
README.md (1)
245-248: New ADR Section Added
The addition of the "ADRs" section with links to the two new ADR documents clearly supports the PR objective of documenting architectural decisions diverging from CAPI contracts. The section is well placed and integrates naturally into the overall documentation.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
246-246: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
247-247: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
docs/adr/002-distribution-version-upgrades.md (2)
1-8: Clear and Comprehensive ADR Document
The ADR document effectively explains the rationale behind using thedistributionVersionfield to drive upgrades and addresses the challenges posed by the differences between OpenShift and standard CAPI versioning. The context, decision, and consequences are outlined in a structured manner that aligns well with the PR objectives.
32-33: Overall ADR Consequences
The "Consequences" section clearly identifies the risks and trade-offs of the current approach. This level of detail will be valuable during upstream discussions and for future reference.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
docs/adr/001-distribution-version.md (3)
7-11: Typo Correction in ContextOn line 9, the text starts with “luster API (CAPI)” instead of “Cluster API (CAPI)”. Please update the typo for clarity.
- luster API (CAPI) contracts rely heavily on the kubernetesVersion field ... + Cluster API (CAPI) contracts rely heavily on the kubernetesVersion field ...
19-21: Punctuation Enhancement in ConsequencesIn the first consequence (line 19), consider inserting a comma after “CAPI contract” for improved readability. For example:
- - This approach diverges from the CAPI contract but is necessary to support OpenShift versioning. + - This approach diverges from the CAPI contract, but is necessary to support OpenShift versioning.🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: Possible missing comma found.
Context: ...approach diverges from the CAPI contract but is necessary to support OpenShift versi...(AI_HYDRA_LEO_MISSING_COMMA)
22-22: File TerminationEnsure the file ends with a newline (if not already) for consistency with POSIX standards.
docs/adr/002-distribution-version-upgrades.md (3)
17-17: Minor Punctuation SuggestionOn line 17, "This is effectively an in-place upgrade" could optionally end with a period for consistency. This is a stylistic choice.
28-31: Determiner Addition for Improved ReadabilityOn line 31, the sentence begins with “- Current approach forces
distributionVersionchanges on the ControlPlane resource...”. Consider adding a determiner to read “forces thedistributionVersionchanges…” for improved grammar:- - Current approach forces `distributionVersion` changes on the ControlPlane resource to represent the whole cluster’s version, which is not ideal. + - Current approach forces the `distributionVersion` changes on the ControlPlane resource to represent the whole cluster’s version, which is not ideal.🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: A determiner appears to be missing. Consider inserting it.
Context: ...m the standard CAPI upgrade contract. - Current approach forcesdistributionVersionc...(AI_EN_LECTOR_MISSING_DETERMINER)
33-34: File TerminationEnsure the file ends with a newline (if not already) for standard compliance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)docs/adr/001-distribution-version.md(1 hunks)docs/adr/002-distribution-version-upgrades.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🪛 LanguageTool
docs/adr/002-distribution-version-upgrades.md
[uncategorized] ~31-~31: A determiner appears to be missing. Consider inserting it.
Context: ...m the standard CAPI upgrade contract. - Current approach forces distributionVersion c...
(AI_EN_LECTOR_MISSING_DETERMINER)
docs/adr/001-distribution-version.md
[uncategorized] ~19-~19: Possible missing comma found.
Context: ...approach diverges from the CAPI contract but is necessary to support OpenShift versi...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (3)
docs/adr/001-distribution-version.md (1)
1-2: Title and Introduction ClarityThe title and header are clear and descriptive. No changes needed here.
docs/adr/002-distribution-version-upgrades.md (2)
1-2: ADR Title ClarityThe title clearly indicates the purpose of the ADR. No modifications are needed.
7-15: Context Section ReviewThe context is well explained. The rationale behind why OpenShift versioning cannot be directly mapped to kubernetesVersion is clearly articulated. No changes are needed.
docs/adr/001-distribution-version.md
Outdated
|
|
||
| ## Context | ||
|
|
||
| luster API (CAPI) contracts rely heavily on the kubernetesVersion field to define Kubernetes versioning and manage upgrades. However, when dealing with OpenShift, there is no direct one-to-one mapping between the kubernetesVersion and OpenShift’s versioning scheme. Because of this, we had to introduce a custom field named `distributionVersion`. |
Signed-off-by: Riccardo Piccoli <rpiccoli@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docs/adr/002-distribution-version-upgrades.md (3)
3-7: Status Section Evaluation
The "Status" section simply marking the ADR as "Proposed" works, although adding a date or version identifier could improve traceability for future revisions.
7-20: Context Section Clarity
The context is well-explained, addressing the challenges in mapping OpenShift to Kubernetes versions and outlining why a centralized, unified approach is necessary. For consistency and increased clarity, consider capitalizing “Kubernetes” throughout the text.
28-33: Consequences Section – Grammar and Clarity Adjustment
The consequences are well-articulated, highlighting important trade-offs. However, on line 31, a determiner appears to be missing. Consider revising the sentence for clarity as follows:- Current approach forces `distributionVersion` changes on the ControlPlane resource to represent the whole cluster’s version, which is not ideal. + Current approach forces the `distributionVersion` field on the ControlPlane resource to represent the entire cluster’s version, which is not ideal.🧰 Tools
🪛 LanguageTool
[uncategorized] ~31-~31: A determiner appears to be missing. Consider inserting it.
Context: ...m the standard CAPI upgrade contract. - Current approach forcesdistributionVersionc...(AI_EN_LECTOR_MISSING_DETERMINER)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)docs/adr/001-distribution-version.md(1 hunks)docs/adr/002-distribution-version-upgrades.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- docs/adr/001-distribution-version.md
🧰 Additional context used
🪛 LanguageTool
docs/adr/002-distribution-version-upgrades.md
[uncategorized] ~31-~31: A determiner appears to be missing. Consider inserting it.
Context: ...m the standard CAPI upgrade contract. - Current approach forces distributionVersion c...
(AI_EN_LECTOR_MISSING_DETERMINER)
🔇 Additional comments (2)
docs/adr/002-distribution-version-upgrades.md (2)
1-1: Review Title Accuracy
The ADR title is clear and descriptive, immediately conveying the focus on usingdistributionVersionto drive upgrades.
21-27: Decision Section Appropriateness
The decision section clearly lists the proposed approaches and the rationale behind them. The bullet points are concise and relevant, effectively summarizing the path forward.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, rccrdpccl 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-merge-bot
bot
merged commit acd2465
into
openshift-assisted:master
Document architectural decisions that move away from CAPI contracts.
Summary by CodeRabbit
README.mdfile, providing structured references to architectural decision records related to distribution versions and upgrades.