Skip to content

Conversation

@csrwng
Copy link
Contributor

@csrwng csrwng commented Feb 11, 2015

Adds a new field to templates that includes a set of labels to apply to all objects during template processing.

@csrwng
Copy link
Contributor Author

csrwng commented Feb 11, 2015

@bparees fyi

@bparees
Copy link
Contributor

bparees commented Feb 11, 2015

@bparees
Copy link
Contributor

bparees commented Feb 11, 2015

awesome! can we get a test case?

@csrwng
Copy link
Contributor Author

csrwng commented Feb 11, 2015

Added test case to template/registry

@bparees
Copy link
Contributor

bparees commented Feb 11, 2015

lgtm, you want anyone else to review or more edits before merging?

Copy link
Contributor

Choose a reason for hiding this comment

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

Labels

Copy link
Contributor

Choose a reason for hiding this comment

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

Why we can't just re-use the ObjectMeta labels? And apply/merge them with labels defined for the items?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mfojtik that seems slightly abusive, particularly once we have a template repository and might want templates to actually have their own labels for their own purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bparees how about making it more explicit then? ItemLabels as they are intended to apply to all 'items' ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mfojtik it's ok with me, but i think @smarterclayton is going to reply with "Labels" again :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, none of these objects should really be in the root of the template - a template should have a section that is specifically for the items, and one specifically for the things to change.

It should really be something like:

{
  "metadata": { // meta about the template },
  "objects": [] // not "items", because then Template is interpreted by generic behavior to be a List
  "spec": {
    "parameters": [], // environment substitution
    "label": {
      "operation": "Add",
      "labels":    {} // these get added
    }
  }
}

We don't have to do that in v1beta1, but for v1beta2 it should be easy to clear the "spec" (or whatever it should be called) in one operation.

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

@@ -16,6 +16,10 @@ type Template struct {
// Optional: Parameters is an array of Parameters used during the
// Template to Config transformation.
Parameters []Parameter json:"parameters,omitempty"
+

  • // Optional: ObjectLabels is a set of labels that are applied to every
  • // object during the Template to Config transformation
  • ObjectLabels map[string]string json:"objectlabels,omitempty"

@mfojtik it's ok with me, but i think @smarterclayton is going to reply with
"Labels" again :)


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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smarterclayton @bparees @mfojtik - if the change suggested above is for v1beta2, then for now should it be called ObjectLabels? or ItemLabels? (I'm ok with either one). We can't call it Labels because it overrides the one in ObjectMetadata.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can call it Labels externally and ObjectLabels internally.

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

@@ -16,6 +16,10 @@ type Template struct {
// Optional: Parameters is an array of Parameters used during the
// Template to Config transformation.
Parameters []Parameter json:"parameters,omitempty"
+

  • // Optional: ObjectLabels is a set of labels that are applied to every
  • // object during the Template to Config transformation
  • ObjectLabels map[string]string json:"objectlabels,omitempty"

@smarterclayton @bparees @mfojtik - if the change suggested above is for
v1beta2, then for now should it be called ObjectLabels? or ItemLabels? (I'm
ok with either one). We can't call it Labels because it overrides the one in
ObjectMetadata.


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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smarterclayton - by externally vs internally do you mean internal api vs versioned api ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the versioned API can be labels (since that's JSON, and in JSON it's clear) and internally you can use ObjectLabels.

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

@@ -16,6 +16,10 @@ type Template struct {
// Optional: Parameters is an array of Parameters used during the
// Template to Config transformation.
Parameters []Parameter json:"parameters,omitempty"
+

  • // Optional: ObjectLabels is a set of labels that are applied to every
  • // object during the Template to Config transformation
  • ObjectLabels map[string]string json:"objectlabels,omitempty"

@smarterclayton - by externally vs internally do you mean internal api vs
versioned api ?


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

@csrwng csrwng force-pushed the template_labels branch 3 times, most recently from 2012291 to c4ec466 Compare February 19, 2015 19:08
@csrwng
Copy link
Contributor Author

csrwng commented Feb 19, 2015

Updated json annotation to use 'labels' for ObjectLabels

@smarterclayton
Copy link
Contributor

Please change the field name in the Go struct in the external version.

@csrwng
Copy link
Contributor Author

csrwng commented Feb 19, 2015

done.

@smarterclayton
Copy link
Contributor

Add it to the Resource printer for templates as a column and to the describer and you're good

@smarterclayton
Copy link
Contributor

Actually, don't put it in the printer. In the describer be sure to associate it with the objects (since labels gets shown at the top

@smarterclayton smarterclayton modified the milestone: 0.4.0 (beta2) Feb 20, 2015
@csrwng csrwng force-pushed the template_labels branch 2 times, most recently from bfdf64e to 608f6ca Compare February 20, 2015 04:41
@csrwng
Copy link
Contributor Author

csrwng commented Feb 20, 2015

@smarterclayton - I'll look into the upstream code to find out why DefaultConvert is overwriting Labels on the base object and not just the one in ObjectMetadata. For now, the ordering of the convert calls is important. I added a comment to that part of the code.

@smarterclayton
Copy link
Contributor

LGTM [merge]

@openshift-bot
Copy link
Contributor

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

@openshift-bot
Copy link
Contributor

Evaluated for origin up to 22541e1

openshift-bot pushed a commit that referenced this pull request Feb 20, 2015
@openshift-bot openshift-bot merged commit 4d6a501 into openshift:master Feb 20, 2015
@csrwng csrwng deleted the template_labels branch July 20, 2015 15:35
jpeeler pushed a commit to jpeeler/origin that referenced this pull request Feb 1, 2018
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.

5 participants