Add support for processing templates #183

zakiva · 2016-08-16T15:04:19Z

This PR adds support for processing templates.
A template allows the user to generate a list of objects with certain labels and parameters. This list can be then used in order to create these objects. So, we would like to support processing the template and return the list of objects to create (as part of the processed template).
Template is currently an Openshift entity, and will be added to k8s here.
cc @abonas @simon3z @zeari

abonas · 2016-08-16T15:20:21Z

are you going to add tests? what is this feature about/link to k8s documentation?

zakiva · 2016-08-16T15:26:31Z

are you going to add tests? what is this feature about/link to k8s documentation?

yes, I marked it with WIP since tests and updating the Readme are still missing. I'll also update the description above.

zakiva · 2016-08-28T12:47:55Z

@moolitayer @zeari Please review

abonas · 2016-08-28T18:49:21Z

Currently the feature is not merged on k8s side and that PR wasn't updated for a while.
So either this support should be added to openshift ruby client, or be blocked from merging here until it's merged in k8s - to be on the safe side for users who are using k8s and not openshift.
cc @simon3z

abonas · 2016-08-28T18:50:01Z

README.md

+
+```ruby
+client.process_template template
+```


any example/rules how a template should look like? any mandatory fields?

simon3z · 2016-08-29T07:55:05Z

Currently the feature is not merged on k8s side and that PR wasn't updated for a while.
So either this support should be added to openshift ruby client, or be blocked from merging here until it's merged in k8s - to be on the safe side for users who are using k8s and not openshift.

Yes, @zakiva this should be move to the openshift client.

zakiva · 2016-08-29T12:13:52Z

moved to openshift client, closing.

zakiva · 2016-09-18T07:58:52Z

@simon3z @zeari @moolitayer reopened, please take another look.

enoodle · 2016-09-18T08:33:54Z

lib/kubeclient/common.rb

+      @headers['Content-Type'] = 'application/json'
+      response = handle_exception do
+        rest_client[ns_prefix + 'processedtemplates']
+        .post(template.to_h.to_json, @headers)


Question: I dont understand how this is different from create_template. They are both using post on the received template hash. Can you please explain how this works?

Both indeed use Post on a template, but while create_template actually creates a template, process_template does not. It is only used to substitute the template parameters and generate their values (if required), in order to get a list of objects to create from the template. Note that we need here a separated method because although both requests are using Post they are still different:

POST /oapi/v1/namespaces/{namespace}/templates for creating a template.

POST /oapi/v1/namespaces/{namespace}/processedtemplates for processing a template.

👍 Thank you

moolitayer

Looks good pending minor comment.

moolitayer · 2016-09-18T16:13:40Z

lib/kubeclient/common.rb

+        .post(template.to_h.to_json, @headers)
+      end
+      result = JSON.parse(response)
+      result


style: replace the above two lines with

JSON.parse(response)

done, thanks.

moolitayer · 2016-09-18T16:46:49Z

lib/kubeclient/common.rb

@@ -405,6 +405,17 @@ def proxy_url(kind, name, port, namespace = '')
      end
    end

+    def process_template(template)
+      ns_prefix = build_namespace_prefix(template[:metadata][:namespace])
+      @headers['Content-Type'] = 'application/json'


Actually we do not want to update @headers except for this request.
Please follow #190 to clean this up.

done, thanks.

simon3z · 2016-09-19T08:03:50Z

lib/kubeclient/common.rb

+        rest_client[ns_prefix + 'processedtemplates']
+        .post(template.to_h.to_json, { 'Content-Type' => 'application/json' }.merge(@headers))
+      end
+      JSON.parse(response)


@zakiva @moolitayer I thought this was taken care of by the rest client.

No and I don't see rest_client has support for that. Our exception handler parses exceptions but other then that we parse the response manually on all calls but get_pod_log. I recently thought about wrapping some of the rest_client logic since it is repetitive. Maybe that belongs there too. Not in this PR though

moolitayer · 2016-09-19T08:20:06Z

LGTM 👍

moolitayer · 2016-09-21T09:33:40Z

Since last comments regarding kubeclinet vs openshift_client from 24 days ago some things changed:

we are looking into replacing openshift_client with a kubeclient with discovery on the openshift endpoint - see openshift_client here
The only real functional difference between the two would be the new process_template method as clients were kept functionally similar up until this point (which is great for us)
Given those facts and that the issue in question is targeted to reach kubernetes at some point our cleanest direction for these two clients would be to add process_templates in kubeclient

@abonas @jonmoter @jimmidyson @nyxcharon @Fryguy @zeari @yaacov @cben @grosser

Any comment on this? We'd like to merge this ASAP

simon3z · 2016-09-21T09:41:27Z

Any comment on this? We'd like to merge this ASAP

LGTM 👍, I haven't reviewed the code closely, maybe there are nits to fix, but I agree with the direction.

zeari · 2016-09-22T12:29:53Z

LGTM 👍

enoodle

👍 LGTM.
I would have made the test a bit more simple but its not very important as it can be easily changed if someone ever finds it as a problem.

enoodle · 2016-09-22T15:34:51Z

test/test_process_template.rb

+    req_body = "{\"metadata\":{\"name\":\"my-template\",\"namespace\":\"default\"},"\
+    "\"kind\":\"Template\",\"apiVersion\":\"v1\",\"objects\":[{\"metadata\":"\
+    "{\"name\":\"${NAME_PREFIX}my-service\"},\"kind\":\"Service\",\"apiVersion\":\"v1\"}],"\
+    "\"parameters\":[{\"name\":\"NAME_PREFIX\",\"value\":\"test/\"}]}"


Is this the best way to create the request body?
Do we really need a request body here? I see that in other tests we just mocked without this and it seems to be enough.

salilgupta1 · 2017-03-01T20:52:14Z

Might be nice to clarify in the README that process_template is not something that is supported by k8s right now.

simon3z · 2017-03-01T20:55:11Z

Might be nice to clarify in the README that process_template is not something that is supported by k8s right now.

Right @salilgupta1, can you or @zakiva send a PR to update the README file?

zakiva changed the title ~~Add support for processing templates~~ [WIP] Add support for processing templates Aug 16, 2016

zakiva force-pushed the add_process_template branch from 5f19132 to ea958ef Compare August 17, 2016 12:44

zakiva force-pushed the add_process_template branch 4 times, most recently from 9110902 to 5e2bd3b Compare August 28, 2016 12:32

zakiva changed the title ~~[WIP] Add support for processing templates~~ Add support for processing templates Aug 28, 2016

abonas reviewed Aug 28, 2016
View reviewed changes

README.md

```ruby

client.process_template template

```

Copy link

Member

abonas Aug 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any example/rules how a template should look like? any mandatory fields?

zakiva closed this Aug 29, 2016

simon3z mentioned this pull request Sep 13, 2016

Support k8s api discovery and group api #185

Merged

zakiva reopened this Sep 18, 2016

zakiva force-pushed the add_process_template branch from 5e2bd3b to bb8d9c9 Compare September 18, 2016 07:54

enoodle reviewed Sep 18, 2016

View reviewed changes

moolitayer reviewed Sep 18, 2016

View reviewed changes

Add support for processing templates

409f4f6

zakiva force-pushed the add_process_template branch from bb8d9c9 to 409f4f6 Compare September 19, 2016 08:00

simon3z reviewed Sep 19, 2016

View reviewed changes

enoodle approved these changes Sep 22, 2016

View reviewed changes

simon3z merged commit 4db928d into ManageIQ:master Sep 23, 2016

zakiva mentioned this pull request Sep 25, 2016

Add support for processing templates abonas/openshift_client#29

Closed

cben mentioned this pull request May 19, 2019

ResourceNotUniqueError on resources.get(api_version='v1', kind='DeploymentConfig') openshift/openshift-restclient-python#284

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for processing templates #183

Add support for processing templates #183

zakiva commented Aug 16, 2016 •

edited

Loading

abonas commented Aug 16, 2016

zakiva commented Aug 16, 2016

zakiva commented Aug 28, 2016

abonas commented Aug 28, 2016

abonas Aug 28, 2016

simon3z commented Aug 29, 2016

zakiva commented Aug 29, 2016

zakiva commented Sep 18, 2016

enoodle Sep 18, 2016

zakiva Sep 18, 2016

enoodle Sep 18, 2016

moolitayer left a comment

moolitayer Sep 18, 2016

zakiva Sep 19, 2016

moolitayer Sep 18, 2016

zakiva Sep 19, 2016

simon3z Sep 19, 2016

moolitayer Sep 19, 2016

moolitayer commented Sep 19, 2016

moolitayer commented Sep 21, 2016 •

edited

Loading

simon3z commented Sep 21, 2016

zeari commented Sep 22, 2016

enoodle left a comment

enoodle Sep 22, 2016 •

edited

Loading

salilgupta1 commented Mar 1, 2017

simon3z commented Mar 1, 2017

Add support for processing templates #183

Add support for processing templates #183

Conversation

zakiva commented Aug 16, 2016 • edited Loading

abonas commented Aug 16, 2016

zakiva commented Aug 16, 2016

zakiva commented Aug 28, 2016

abonas commented Aug 28, 2016

Choose a reason for hiding this comment

simon3z commented Aug 29, 2016

zakiva commented Aug 29, 2016

zakiva commented Sep 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Sep 19, 2016

moolitayer commented Sep 21, 2016 • edited Loading

simon3z commented Sep 21, 2016

zeari commented Sep 22, 2016

enoodle left a comment

Choose a reason for hiding this comment

enoodle Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

salilgupta1 commented Mar 1, 2017

simon3z commented Mar 1, 2017

zakiva commented Aug 16, 2016 •

edited

Loading

moolitayer commented Sep 21, 2016 •

edited

Loading

enoodle Sep 22, 2016 •

edited

Loading