-
Notifications
You must be signed in to change notification settings - Fork 213
api: Move CVOConfig to final name and location #44
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
Conversation
config.openshift.io will be the API group for all top level cluster config, including ClusterOperator and CVOConfig, as well as other component level config (image, network, cloud). This commit does a rename in place of clusterversion.openshift.io to config.openshift.io in this operator.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarterclayton 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 |
|
when this merges we also need to merge openshift/installer#549 |
|
@deads2k @abhinavdahiya @crawford we've all talked about aspects of this before, if we're not on the same page completely I'll keep adding detail until we are. This only renames CVOConfig and moves groups. |
| scheme.AddKnownTypes(SchemeGroupVersion, | ||
| &CVOConfig{}, | ||
| &ClusterVersion{}, | ||
| &CVOStatus{}, |
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 stands out now.
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.
Yeah, I think the spec/status split discussion can happen as a second step here. I will create a PR that has those additional commits so we can look at it.
| func addKnownTypes(scheme *runtime.Scheme) error { | ||
| scheme.AddKnownTypes(SchemeGroupVersion, | ||
| &CVOConfig{}, | ||
| &ClusterVersion{}, |
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.
pre-existing, but where is the list?
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.
Added
|
I'm in favor of the change. I think you accidentally moved a second type with this type though. |
|
It's not easy to make the installer able to tolerate both CRDs, so the best option I can see is to get review and deliver the rename to the CRD into both repos at once. |
This commit renames CVOConfig to ClusterVersion and regenerates the clients, but does not change any of the fields or usages of the object.
10ce824 to
cd92893
Compare
|
lgtm will leave tagging to @abhinavdahiya or @crawford |
|
Closing in favor of #45 which contains this change is approved by abhinav |
As discussed in previous review sessions, we want to have all core configuration api objects in the same API group, and we also want to rename CVOConfig to ClusterVersion. This PR changes both - the proposed name for the group is
config.openshift.ioas in, "the config for an openshift cluster".Reasons for wanting ClusterVersion as a name:
Reasons for wanting
config.openshift.ioas a group name:Reasons for wanting one API group for ClusterVersion, ClusterOperator, and top level config (Image, Network)
Reasons for moving ClusterOperator into
config.openshift.ioChanges that have been discussed and may influence how people feel about this change, but are not part of this PR:
ClusterVersionfield naming to match terminology we are using else whereconfig.openshift.ioOne example possible final state (assuming we did all above):