-
Notifications
You must be signed in to change notification settings - Fork 39.4k
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
Template Api Support #25293
Template Api Support #25293
Conversation
cc @bparees Since you worked on the original design proposal it might be helpful for you to take a look at the api cc @kubernetes/kube-api |
All commits including "fixup" and earlier are from another PR that this depends on. No need to review them. |
|
||
// Optional: Value holds the Parameter data. If specified, the generator | ||
// will be ignored. The value replaces all occurrences of the Parameter | ||
// ${Name} expression during the Template to Config transformation. |
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.
there will no generators in the k8s implementation, and the substitution string/syntax will be
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.
Done
the api objects look good, i'm a little fuzzier on the rest api, particularly around processing templates. In the spec we talked about 2 key aspects:
it looks like what's defined here is: post parameters, get back a list of objects. I think we may still want the "post a template" input option so users can process a template without having to first "create" it. (if that's there and i missed it, sorry) for the response object, i think just returning the list of objects (instead of the full template) is right. |
cc: @mfojtik |
Sounds good. I will talk with @lavalamp about how to best expose this in api. Do you have a preference (e.g. different process endpoints for different types, a single request that wraps both input types, etc)? |
a separate endpoint makes sense to me... logically you'd then have:
|
@bparees Daniel agreed the second endpoint it sensible way of doing this. Since this is the second to last week of coding for 1.3 I'd like to minimize the scope to an MVP before addressing anything else. Would you be ok with me adding a TODO for adding the second api endpoint once the first endpoint is complete? |
sounds reasonable |
Making the processtemplate endpoint cluster-scoped (no namespace) would make sense as long as there's no fan-out (calls to other resources). Maybe too late to change now, but "config" might be a good group name for both template resources. |
@lavalamp How hard would it be to create a config/v1/beta1 api group? Do you think it is worth it / Does it put this feature at risk for 1.3? |
It requires a bunch of counter-intuitive boilerplate, basically, although On Mon, May 9, 2016 at 2:06 PM, Phillip Wittrock notifications@github.com
|
Leaving in extensions SGTM then. Just getting the api hooked up was mainly copy paste, but it took me longer than expected to figure out exactly how it needed to be done. |
PTAL |
Squashed |
The validation and processing logic will be in #25622 |
PTAL. |
unversioned.TypeMeta `json:",inline"` | ||
|
||
// Name is the name of the Template to be processed. | ||
Name string `json:"name"` |
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.
Looks like Brian is correct. Consider just using object meta instead, to be consistent with Binding.
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.
ObjectMeta has a bunch of stuff like namespace and nameGenerator that I don't think we want. This is consistent with extensions/v1beta.DeploymentRollback and I think I prefer it. WDYT?
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.
We use ObjectMeta in many places for virtual resources (like
SubjectAccessReview).
On Tue, May 17, 2016 at 2:53 PM, Daniel Smith notifications@github.com
wrote:
In pkg/apis/extensions/types.go
#25293 (comment)
:
- Required bool
json:"required,omitempty"
- // Type is the type that the parameter value must be parsed to. Type may be one of
- // 'string', 'integer', 'boolean', or 'base64'.
- // Type is used by clients to provide validation of user input and direction to users.
- // Parameters used to define integer or boolean fields (e.g. replicaCount) should have the
- // Type set to integer or boolean accordingly.
- Type string
json:"type,omitempty"
+}
+// TemplateParameters contains the substitution parameter overrides when processing a Template
+type TemplateParameters struct {
- unversioned.TypeMeta
json:",inline"
- // Name is the name of the Template to be processed.
- Name string
json:"name"
Looks like Brian is correct. Consider just using object meta instead, to
be consistent with Binding.—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/25293/files/58c7e4126d33836cce7082e9aa04f6818be06129..c7a56db1935dabe7aea0399ebfbed3cf5661c927#r63581936
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.
Never mind, that was a lie.
On Tue, May 17, 2016 at 3:43 PM, Clayton Coleman ccoleman@redhat.com
wrote:
We use ObjectMeta in many places for virtual resources (like
SubjectAccessReview).On Tue, May 17, 2016 at 2:53 PM, Daniel Smith notifications@github.com
wrote:In pkg/apis/extensions/types.go
#25293 (comment)
:
- Required bool
json:"required,omitempty"
- // Type is the type that the parameter value must be parsed to. Type may be one of
- // 'string', 'integer', 'boolean', or 'base64'.
- // Type is used by clients to provide validation of user input and direction to users.
- // Parameters used to define integer or boolean fields (e.g. replicaCount) should have the
- // Type set to integer or boolean accordingly.
- Type string
json:"type,omitempty"
+}
+// TemplateParameters contains the substitution parameter overrides when processing a Template
+type TemplateParameters struct {
- unversioned.TypeMeta
json:",inline"
- // Name is the name of the Template to be processed.
- Name string
json:"name"
Looks like Brian is correct. Consider just using object meta instead, to
be consistent with Binding.—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/25293/files/58c7e4126d33836cce7082e9aa04f6818be06129..c7a56db1935dabe7aea0399ebfbed3cf5661c927#r63581936
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.
I don't want any fields here that aren't actually needed/supported.
In retrospect, ObjectMeta was confusing, since most of the fields aren't usable, and it refers to the parent resource rather than the subresource. A subset of ObjectReference probably would have been more appropriate.
I'm open to anything that would make programmatic clients easier to write (e.g., used with the discovery API), but I don't envision users writing declarative config files for subresources.
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.
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.
@bparees @pwittrock I don't understand how the kubectl proposal relates to this API discussion.
AIUI, the problem we have is that, unlike admission control, REST handlers aren't passed all of the information available from the path in the request context. If we fix that problem, then the information doesn't need to be explicitly represented in the body schema for imperative subresources that aren't serialized in manifests nor in etcd.
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.
@bgrant0607 You mean if we post the parameters to an api endpoint like:
api/templates/<templatename>/process
the rest handler can't determine "templatename"?
I would tend to agree that it makes more sense to fix that problem than to require the parameters struct include the templatename. Especially since i might want to use the same parameters resource definition with multiple templates.
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.
@bparees Yes, that's exactly what I mean. I haven't looked at the code recently, but I recall it being a problem for /binding, /scale, and /rollback.
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.
@bgrant0607 definitely sounds like something we want to fix if this is the fourth endpoint to have a use for that info.
This should be pushed to 1.4. Will still update with your comments @lavalamp. |
@pwittrock PR needs rebase |
@pwittrock do you still intend to try to get this in 1.4? |
@ncdc I don't think so. Is this something that you are waiting for or were planning on? |
I think we need to discuss in the apps sig about the timing and implementation. It's important to us, but there's a lot of important stuff going on right now. I think the clearest overarching theme for Kube right now is "getting started" and there are things in templates that do address that, but it'll be a balancing on other timing. |
@smarterclayton Sounds good to me. I am hoping that Helm will be able to address some of the same things as Templates for the getting started experience. |
GCE e2e build/test passed for commit 5124918. |
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. |
This PR hasn't been active in 60 days. It will be closed in 29 days (Nov 8, 2016). You can add 'keep-open' label to prevent this from happening, or add a comment to keep it open another 90 days |
This change is