Skip to content
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

Perform template processing on external values #1959

Merged
merged 3 commits into from
Apr 29, 2015

Conversation

smarterclayton
Copy link
Contributor

Templates currently perform a conversion, which populates default
values and also runs afoul of rules in conversion which may delete/alter/validate
the values we want to templatize.

This change alters the behavior of api.List to deserialize objects into
runtime.Unknown rather than performing the full conversion. It also adds
a new runtime.Unstructured type that can be manipulated directly.

@smarterclayton smarterclayton force-pushed the fix_templates branch 3 times, most recently from 2288ed4 to 3de701d Compare April 29, 2015 03:00
@smarterclayton
Copy link
Contributor Author

@mfojtik or @deads2k, you get hit with the review stick unfortunately. The upstream will probably be created tomorrow.

@smarterclayton
Copy link
Contributor Author

[test]

@smarterclayton
Copy link
Contributor Author

Upstream is kubernetes/kubernetes#7490

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1931/)

@mfojtik
Copy link
Contributor

mfojtik commented Apr 29, 2015

@smarterclayton this looks awesome, thanks! LGTM

// objects still have functioning TypeMeta features-- kind, version, etc.
// TODO: Make this object have easy access to field based accessors and settors for
// metadata and field mutatation.
type Unstructured struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we keep both Unknown and Unstructured? Seems like Unstructured is a more convenient form that is equivalent in power.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unknown is much less expensive and is essentially "an opaque blob". Not everyone who wants to use objects needs the processing Unstructured has.

----- Original Message -----

@@ -116,3 +116,17 @@ type Unknown struct {
}

func (*Unknown) IsAnAPIObject() {}
+
+// Unstructured allows objects that do not have Golang structs registered
to be manipulated
+// generically. This can be used to deal with the API objects from a
plug-in. Unstructured
+// objects still have functioning TypeMeta features-- kind, version, etc.
+// TODO: Make this object have easy access to field based accessors and
settors for
+// metadata and field mutatation.
+type Unstructured struct {

Why would we keep both Unknown and Unstructured? Seems like
Unstructured is a more convenient form that is equivalent in power.


Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1959/files#r29328825

Copy link
Contributor

Choose a reason for hiding this comment

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

Unknown is much less expensive

That's not obvious to me. Don't you have to parse the json anyway, effectively creating the map, until you find hits for the kind and version regardless?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

json.Decoder has a token scanner that it uses to identify fields. You perform O(N) less object creation. Decode time and memory use is linear with number of fields in your destination structure.

----- Original Message -----

@@ -116,3 +116,17 @@ type Unknown struct {
}

func (*Unknown) IsAnAPIObject() {}
+
+// Unstructured allows objects that do not have Golang structs registered
to be manipulated
+// generically. This can be used to deal with the API objects from a
plug-in. Unstructured
+// objects still have functioning TypeMeta features-- kind, version, etc.
+// TODO: Make this object have easy access to field based accessors and
settors for
+// metadata and field mutatation.
+type Unstructured struct {

Unknown is much less expensive

That's not obvious to me. Don't you have to parse the json anyway,
effectively creating the map, until you find hits for the kind and
version regardless?


Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1959/files#r29335924

@deads2k
Copy link
Contributor

deads2k commented Apr 29, 2015

Oh, ooops. Just realized that I've been commenting in the wrong repo. Should I move them?

@@ -138,6 +139,7 @@ func OverwriteBootstrapPolicy(etcdHelper tools.EtcdHelper, masterNamespace, poli
if !ok {
return errors.New("policy must be contained in a template. One can be created with '" + createBootstrapPolicyCommand + "'.")
}
runtime.DecodeList(template.Objects, kapi.Scheme)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary. Given the danger, we only whitelist types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Necessary, nothing is converted otherwise.

Treat []runtime.Object with more care, require user intervention
to transform values
Allow templates to handle map[string]interface{}
@deads2k
Copy link
Contributor

deads2k commented Apr 29, 2015

lgtm. Merge at will (not sure if you want to wait for upstream comments).

@smarterclayton
Copy link
Contributor Author

Will grab those in rebase. [merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1718/) (Image: devenv-fedora_1393)

@openshift-bot
Copy link
Contributor

Evaluated for origin up to 5576aae

openshift-bot pushed a commit that referenced this pull request Apr 29, 2015
@openshift-bot openshift-bot merged commit 59012cd into openshift:master Apr 29, 2015
@smarterclayton smarterclayton deleted the fix_templates branch May 18, 2015 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants