Add user selectable capabilities in installconfig #5605
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
pkg/types/installconfig.go
Outdated
There was a problem hiding this comment.
Should this be optional? Are we going to omit the capabilities configuration from the ClusterVersion resource when this is omitted? I would expect that we would still set a default value in the ClusterVersion resource.
There was a problem hiding this comment.
Yes. This should be optional. I've added the macro comment.
There was a problem hiding this comment.
OK. Optional does make sense. But I do think we want to populate with the default, even when the entire Capabilities has been omitted. I am also leaning towards if the user provided any values for Include or Exclude, then they are also required to make an explicit decision about InclusionDefault (despite it being redundant with whether they chose Include or Exclude).
There was a problem hiding this comment.
The design has changed since the enhancement and I've updated the PR with the new ideas. PTAL
|
/retest |
openshift-ci
bot
added
needs-rebase
pkg/types/installconfig.go
Outdated
There was a problem hiding this comment.
Is it useful to declare your own type? I'd have expected you to use *configv1.ClusterVersionCapabilitiesSpec as the type for the new InstallConfig property.
There was a problem hiding this comment.
+1. Will remove. I was working myself backwards into reusing all the defs from configv1 but missed this one.
There was a problem hiding this comment.
The reason that we want to use our own type is because it allows us to control changes to meet our forward-compatibility guarantees.
There was a problem hiding this comment.
Won't openshift/api have those same forward-compat commitments?
|
Looks good to me :). I'll let the installer maintainers take a look (and I've filed #5644 to drop myself from the approver set). |
Fix the explain print output to include capabilities
… user-selectable capabilities
|
@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. |
|
Closing this PR in favor of #5645 |
3 participants
This is a partial implementation of user selectable install solutions
This PR only adds install-config configuration to manage capabilities.
Edit: New and additional details improving on the enhancement can be found here and is incorporated in this PR