[WIP/PoC] builds api group #12868
[WIP/PoC] builds api group #12868
Conversation
sttts
force-pushed
the
apigroups
branch
2 times, most recently
from
0029088
to
27aaa6f
Compare
|
/cc |
pkg/cmd/server/origin/master.go
Outdated
| "builds": storage["builds"], | ||
| "builds/clone": storage["builds/clone"], | ||
| "builds/log": storage["builds/log"], | ||
| "builds/details": storage["builds/details"], |
There was a problem hiding this comment.
need buildConfigs, etc. I wonder if we want to move this somewhere more visible or have a test that checks if we did this for all storage endpoints (in case you add new endpoint and forged to register it).
|
@deads2k guess if we have this for all groups, we can drop the legacy |
I think it depends on how we deal with the controllers, but probably. |
| jenkinsEnabled: boolptr(true), | ||
| validateClients: noAction, | ||
| }, | ||
| { | ||
| name: "subresource", | ||
| attributes: admission.NewAttributesRecord(enableBuild, nil, unversioned.GroupVersionKind{}, "namespace", "name", buildapi.SchemeGroupVersion.WithResource("builds"), "subresource", admission.Create, &user.DefaultInfo{}), | ||
| attributes: admission.NewAttributesRecord(enableBuild, nil, unversioned.GroupVersionKind{}, "namespace", "name", buildapi.LegacySchemeGroupVersion.WithResource("builds"), "subresource", admission.Create, &user.DefaultInfo{}), |
There was a problem hiding this comment.
This means we'll need a sweep of admission plugins to make sure they aren't skipping resources they should check based on group.
| "github.com/openshift/origin/pkg/build/api/v1" | ||
| ) | ||
|
|
||
| const importPrefix = "github.com/openshift/origin/pkg/build/api" |
There was a problem hiding this comment.
This may end up being a two stage process. As I recall, some of the client generators freak out when the last step of the package isn't the group
| const GroupName = "" | ||
| const FutureGroupName = "build.openshift.io" | ||
| const LegacyGroupName = "" | ||
| const GroupName = "build.openshift.io" |
There was a problem hiding this comment.
This is likely to have interesting effects on bootstrap policy. I think we actually get away with for reconciliation purposes since new clusters won't care and old clusters will be additive only. @liggitt that's neat, isn't it?
There was a problem hiding this comment.
yeah, currently only cluster admin can create resources in group... we will have to duplicate bootstrap policy for all grouped resources?
| @@ -50,7 +50,7 @@ var ( | |||
| securityGroup = securityapi.GroupName | |||
| storageGroup = storage.GroupName | |||
| authzGroup = authorizationapi.GroupName | |||
| buildGroup = buildapi.GroupName | |||
| buildGroup = buildapi.LegacyGroupName | |||
There was a problem hiding this comment.
ah, you want both. Might make buildGroups and use it below. Groups(buildGroups...)
There was a problem hiding this comment.
It's just a rename now. We know that we have to duplicate the rules there.
| @@ -200,7 +202,8 @@ func (c *MasterConfig) RunInProxyMode(proxy *kubernetes.ProxyConfig, assetConfig | |||
| messages = append(messages, fmt.Sprintf("Started OpenAPI Schema at %%s%s", openAPIServePath)) | |||
|
|
|||
| // install origin handlers | |||
| c.InstallProtectedAPI(container) | |||
| // REBASE: delete proxy mode or make the following work | |||
There was a problem hiding this comment.
@smarterclayton we get to remove it, right? Never produced a reliable cluster and no the direction we want to proceed in.
pkg/cmd/server/origin/master.go
Outdated
| // initialize OpenShift API | ||
| storage := c.GetRestStorage() | ||
|
|
||
| apiGroupInfo := genericapiserver.NewDefaultAPIGroupInfo(buildapi.GroupName) | ||
| //if apiResourceConfigSource.AnyResourcesForVersionEnabled(rbacapiv1alpha1.SchemeGroupVersion) { | ||
| apiGroupInfo.VersionedResourcesStorageMap[buildapiv1.SchemeGroupVersion.Version] = map[string]rest.Storage{ |
There was a problem hiding this comment.
Not that this has to do it, but I like the pattern I made upstream with these being built closer to the registry. I don't want the interface I made for it (that was a mistake), but the code locality worked out nicely.
There was a problem hiding this comment.
yeah, we should follow the upstream pattern here. This was just the easiest way to get the PoC going without refactoring half of origin.
|
does this actually work? It's a lot cleaner than I would have expected. |
|
@deads2k it does (but need bootstrap updates)... i mean the "builds" created by build controller are in the new api group;..... also oc get builds hit the new endpoint and all builds returned have new api group in version. |
sttts
force-pushed
the
apigroups
branch
2 times, most recently
from
7622e50
to
e2fb448
Compare
|
@liggitt @smarterclayton ptal. Do we miss anything if we go this route? |
does
only when returned from the new API, right? do we serialize buildconfigs into build annotations? which api version are they serialized as? |
|
@liggitt yes, only against old server and yes. we do serialize them (as well as deployment configs in RC)... I checked DC in RC and it was using "legacy" version. (I think we can change that) |
If we do, we break old clients, right? |
|
@liggitt yes. we don't have to. |
|
Origin Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again |
openshift-bot
added
the
needs-rebase
|
Closing this in favor of: #12986 |
/cc @mfojtik