-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
make bootstrap template compatible with 1.5 clusters #16207
make bootstrap template compatible with 1.5 clusters #16207
Conversation
@@ -40,13 +39,13 @@ func DefaultTemplate() *templateapi.Template { | |||
oapi.OpenShiftDisplayName: "${" + ProjectDisplayNameParam + "}", | |||
projectapi.ProjectRequester: "${" + ProjectRequesterParam + "}", | |||
} | |||
if err := templateapi.AddObjectsToTemplate(ret, []runtime.Object{project}, projectapiv1.SchemeGroupVersion); err != nil { | |||
if err := templateapi.AddObjectsToTemplate(ret, []runtime.Object{project}, latest.Version); err != nil { |
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 is used for a server to loopback to itself. I don't think I care about making compatible two releases back.
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.
Ah, this is against 3.6.
Can you do this in the CLI hook instead of here?
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 should just be projectapiv1.LegacySchemeGroupVersion
for 3.6 and earlier. I have no idea what you mean by CLI hook.
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 should just be projectapiv1.LegacySchemeGroupVersion for 3.6 and earlier. I have no idea what you mean by CLI hook.
The CLI has a command dumping this where they want to serialize to old objects. Where this is used by the apiserver, we don't care.
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.
@deads2k I am attempting to convert template objects after making a call to delegated.DefaultTemplate()
here, however I am unable to use kapi.Scheme.ConvertToVersion
as it requires pointers as input. Template objects are originally pointers, but are then wrapped in a runtime.encodable when they are added to a template.
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.
@deads2k I am attempting to convert template objects after making a call to delegated.DefaultTemplate() here, however I am unable to use kapi.Scheme.ConvertToVersion as it requires pointers as input. Template objects are originally pointers, but are then wrapped in a runtime.encodable when they are added to a template.
Ok. As long as it stays in 3.6 and doesn't move to 3.7, this is ok. In 3.7 we're using RBAC resources
3195bc7
to
78c46a4
Compare
Thanks, updated the default template to use LegacySchemeGroupVersions |
/lgtm @juanvallejo you need to backport this to OSE. |
Thanks, will do |
/approve but only in previous releases. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, enj, juanvallejo The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/test unit |
/test extended_conformance_install_update |
Automatic merge from submit-queue |
Related BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1486775
$ oc adm create-bootstrap-project-template
generates a template with group versioned resources that are incompatible when used against a 1.5 / 3.5 cluster.cc @openshift/cli-review @mfojtik @enj