-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add in-repo documentation for user-selectable capabilities #5732
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 in-repo documentation for user-selectable capabilities #5732
Conversation
|
/cc @wking |
docs/user/customization.md
Outdated
| ### Capabilities | ||
|
|
||
| * `baselineCapabilitySet` (optional string): Selects an initial set of optional capabilities to enable. Valid values are `vCurrent` (the default), `v4.11` and `None`. | ||
| * `additionalEnabledCapabilities` (optional array of strings): Extends the set of managed capabilities beyond the baseline defined in `baselineCapabilitySet`. Default is an empty set. Valid values are `openshift-samples` and `baremetal`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You validate these, right? Have you bumped your vendored openshift/api to pull in the baremetal addition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the openshift/api was vendored recently and it does contain Baremetal.
installer/vendor/github.com/openshift/api/config/v1/types_cluster_version.go
Lines 267 to 277 in 0614a6d
| var ClusterVersionCapabilitySets = map[ClusterVersionCapabilitySet][]ClusterVersionCapability{ | |
| ClusterVersionCapabilitySetNone: {}, | |
| ClusterVersionCapabilitySet4_11: { | |
| ClusterVersionCapabilityOpenShiftSamples, | |
| ClusterVersionCapabilityBaremetal, | |
| }, | |
| ClusterVersionCapabilitySetCurrent: { | |
| ClusterVersionCapabilityOpenShiftSamples, | |
| ClusterVersionCapabilityBaremetal, | |
| }, | |
| } |
We also iterate over all caps from all sets to validate the valid entries.
installer/pkg/types/validation/installconfig.go
Lines 727 to 733 in 0614a6d
| // Create sets of all capability sets and *all* available capabilities across those capability sets | |
| for baselineSet, capabilities := range configv1.ClusterVersionCapabilitySets { | |
| allCapabilitySets.Insert(string(baselineSet)) | |
| for _, capability := range capabilities { | |
| allAvailableCapabilities.Insert(string(capability)) | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line will need a bump for
marketplacefor #5755. If #5755 lands first, then this PR would need the bump. If this PR lands first, then #5755 would need the bump. Or a third follow up PR could come in with the bump behind both this PR and #5755.
My guess is probably option 3. I'll send a follow up PR with new vendoring and an update to this doc, which should be the norm anyway going forward. Thanks.
|
Example, please! |
docs/user/customization.md
Outdated
| * `pullSecret` (required string): The secret to use when pulling images. | ||
| * `sshKey` (optional string): The public Secure Shell (SSH) key to provide access to instances. | ||
|
|
||
| ### Capabilities | ||
|
|
||
| * `baselineCapabilitySet` (optional string): Selects an initial set of optional capabilities to enable. Valid values are `vCurrent` (the default), `v4.11` and `None`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, as a rule, does vCurrent, include the list of optional capabilities by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vCurrent has a list of baseline capabilities that get installed by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is worth mentioning if the default (vCurrent) value for baselineCapabilitySet already includes all optional capabilities or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ClusterVersionCapabilitySetCurrent is the recommended set of optional capabilities to enable for the cluster's current version of OpenShift.
We're currently recommending all three caps in vCurrent, but there is no commitment to continuing to include those caps or to always adding each new cap.
I'm not installer, so I have no stake in how much openshift/api context folks feel like maintaining here. If it was me, I'd just point at the floating docs.
|
I think this is complete except for the reference to the https://pkg.go.dev/github.com/openshift/api/config/v1#ClusterVersionCapability as mentioned in #5732 (comment). |
Use floating docs links for capabilities and capability sets
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/skip |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
11 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/override ci/prow/e2e-metal-ipi-ovn-ipv6 |
|
@patrickdillon: Overrode contexts on behalf of patrickdillon: ci/prow/e2e-metal-ipi-ovn-ipv6 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
16 similar comments
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
@kirankt: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
|
/override ci/prow/e2e-metal-ipi-ovn-ipv6 |
|
@patrickdillon: Overrode contexts on behalf of patrickdillon: ci/prow/e2e-metal-ipi-ovn-ipv6 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/refresh |
Add in-repo documentation for user-selectable capabilities