Allow to define several API groups#157
Allow to define several API groups#157openshift-merge-robot merged 1 commit intoopenshift:masterfrom
Conversation
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
I understood from slack that we would be adding versioning to this, but it doesn't seem to be handled yet? What's the plan here?
Fedosin
force-pushed
the
apigroups
branch
3 times, most recently
from
9e69b21 to
2495880
Compare
main.go
Outdated
| if len(apiGroupVersions) > 0 { | ||
| // Parsing API Group Versions | ||
| for _, apiGroupVersion := range apiGroupVersions { | ||
| parsedAPIGroupVersion, err := schema.ParseGroupVersion(apiGroupVersion) |
There was a problem hiding this comment.
This parse function assumes that if you haven't got a slash, the entire string is the version, that's not our intended behaviour right?
There was a problem hiding this comment.
If that is our intended behaviour, please document it within the flag
There was a problem hiding this comment.
Yep, I updated the patch - now a string without / means the group, not the version. It is described in the description
|
Can you please clarify on the desc what's the use case that motivates this change? |
The versioning motivation is because of introducing new v1 resources within machine.openshift.io, I had originally raised #156 The multiple groups is because, at the moment, having a CAPI and MAPI CSR approver causes lots of errors in the logs. Half the CSRs cannot be approved by one or the other, so if we want to reduce the errors, we need to just have a single CMA within the cluster that can handle both types |
|
I'm happy with the code changes, but would like to check with @enxebre about his concerns before we merge |
|
ok thanks I see, why did we put this APIs in v1 https://github.com/openshift/api/tree/master/machine/v1 rather than being with the core types in v1beta1? |
|
@Fedosin can you update the PR so that the CMA deployed on HA clusters has the correct flag? That file is in the same folder |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed 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 |
openshift-ci
bot
added
the
approved
we've been asked to not bring new API objects into the v1beta1 version. that's why we created the alibaba providerspec in the v1, and also for the placement groups as well. as i understand it, the api team does not want to introduce new resources in the v1beta1 version, and i gather this has to do with our support life cycle around APIs that are not v1. you can see David's explanation (which is much better than mine) here: openshift/api#1045 (comment) |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
/lgtm cancel
This is broken right now as both flags are set (the old flag needs its default removed)
main.go
Outdated
| if err := validateAPIGroup(APIGroup); err != nil { | ||
| klog.Fatalf(err.Error()) | ||
| // Deprecated options | ||
| flagSet.StringVar(&apiGroup, "apigroup", "machine.openshift.io", "API group for machines") |
There was a problem hiding this comment.
This should now have an empty value else you can't set the new flag
| flagSet.StringVar(&apiGroup, "apigroup", "machine.openshift.io", "API group for machines") | |
| flagSet.StringVar(&apiGroup, "apigroup", "", "API group for machines") |
Currently approver can work with just one API group, which is defined by --apigroup option. This patch allows to set several API groups, so that the component is able to approve certificates for machines created by various APIs. Deprecations: --apigroup is deprecated in favor of --apigroupversion option New options: --api-group-version allows to specify both API group and version. This option can be given multiple times.
|
/lgtm |
|
/retest-required Please review the full test history for this PR and help us cut down flakes. |
3 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. |
|
@Fedosin: 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. |
6 participants
Currently approver can work with just one API group, which is defined by --apigroup option. This patch allows to set several API groups, so that the component is able to approve certificates for machines created by various APIs.
For OpenShift it means that we don't have to deploy 2 approvers: one for MAPI and another for CAPI. Instead we can use one instance that manages them all.
Also, using of api-group-version instead of just api-group allows us to define what API version we'd like to use. It's important because In openshift/machine-api-operator#992, we are introducing a new CRD which is in the machine.openshift.io/v1 group. This means that the server preferred version of the API is now v1 and not v1beta1. Because Machines don't yet exist in the v1 group, this means the CMA is currently broken on that PR.
If we were to merge openshift/machine-api-operator#992 we would break the CMA across openshift. To mitigate this, we must allow the version to be set manually as a part of api-group-version, and only rely on discovery when the preferred version is not set there.
Deprecations:
--apigroupis deprecated in favor of--apigroupversionoptionNew options:
--api-group-versionallows to specify both API group and version. This option can be given multiple times.