Container Template: Add object_labels #15406

zakiva · 2017-06-20T13:58:25Z

Changes included:

~~Adding object_labels column to container_templates table~~ (Extracted), parsing this field is added in Container Template: Parse object_labels manageiq-providers-openshift#25.
~~Allowing the instantiate method to receive labels as an argument so that each created object will be tagged with these labels during instantiation~~. moved to Container Template: Allow instantiation with object_labels manageiq-providers-kubernetes#98
Serializing the object_labels attribute.

zakiva · 2017-06-20T14:17:28Z

@miq-bot add_label providers/containers, enhancement

zakiva · 2017-06-20T14:17:59Z

@lfu This will allow sending the instantiate method the template.object_labels (hash) merged with additional key-value pairs as you'll need. Please review.
cc @simon3z

simon3z · 2017-06-20T20:57:25Z

@zakiva aren't labels a one-to-many relationship to custom_attributes for other entities? Why to implement it differently here?

zakiva · 2017-06-21T08:44:49Z

@zakiva aren't labels a one-to-many relationship to custom_attributes for other entities? Why to implement it differently here?

Template also has one-to-many relationship to custom_attributes for its labels.
The object_labels added here are quite different, we save them in the DB in order to use them when instantiating the template (same as we do for template.objects).
I can still implement them as custom_attributes just with a different section but this means that we'll need to:

Parse the labels (hash) from the inventory and save them as custom_attributes when refreshing.
Parse the custom_attributes from the DB and convert back into hash before sending to Openshift when instantiating the template.

simon3z · 2017-06-21T09:11:28Z

@zakiva then these labels seems instantiation-time labels that we shouldn't persist in the database.

miq-bot · 2017-06-22T17:36:45Z

This pull request is not mergeable. Please rebase and repush.

Fryguy · 2017-06-22T17:46:32Z

Database migrations have now been moved to the https://github.com/ManageIQ/manageiq-schema repo. Please see http://talk.manageiq.org/t/new-split-repo-manageiq-schema/2478 for instructions on how to transfer your database migrations. If this PR contains only migrations, I will leave it open for a short time during the transition, after which I will close this if it has not been moved over.

lfu · 2017-06-22T21:53:13Z

@zakiva Maybe these user specified labels should be saved the same way as the user specified parameters? These parameters currently are saved in Service/ServiceTemplate.

simon3z · 2017-07-04T12:28:17Z

@zakiva @lfu let me know if you need to move this forward (btw it now requires the migration to be moved to the manageiq-schema repo).
I am available for reviews/suggestions, etc.

zakiva · 2017-07-05T08:37:17Z

it now requires the migration to be moved to the manageiq-schema repo

@simon3z done, please take a look

lfu · 2017-07-05T13:23:33Z

app/models/container_template.rb

@@ -26,15 +27,16 @@ class ContainerTemplate < ApplicationRecord
    "Service"               => ContainerService,
  }.freeze

-  def instantiate(params, project = nil)
+  def instantiate(params, labels, project = nil)


@zakiva Is it better to make labels an optional parameter?

we can use template.object_labels as default value, if you prefer? (I thought that we always want to add a unique template label to mark the objects)

I don't mind leave it the same as for template parameters for now.

lfu · 2017-07-05T13:39:43Z

@zakiva If I understand it correctly, the user input instantiation-time labels will be saved in service if implemented later, and those specified in the template will be saved as object_labels in container_templates in DB.
What is the purpose for the labels saved as custom_attributes for container_templates? Are they still needed?

zakiva · 2017-07-05T14:38:52Z

What is the purpose for the labels saved as custom_attributes for container_templates? Are they still needed?

Those are labels that we save for the template as part of its metadata, as we do for the rest of the container objects.

The new object_labels added here are different. We use them so each created object will be tagged with these labels during instantiation (the labels will then be saved as custom_attributes of the related object).

lfu

Can we add some tests?

zakiva · 2017-07-18T10:59:31Z

Can we add some tests?

@lfu added in ManageIQ/manageiq-providers-openshift#37

zakiva · 2017-07-18T14:38:46Z

@miq-bot rm_label sql migration

miq-bot · 2017-08-02T17:57:03Z

This pull request is not mergeable. Please rebase and repush.

miq-bot · 2017-08-21T12:57:56Z

Checked commit zakiva@c00ba9e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

cben · 2017-09-03T19:12:26Z

LGTM.

Using custom_attributes (with new section) could make sense for consistency, but for this use case both are fine. We don't need to search by them.
Refresh performance wise, a serialized column should be OK, possibly better than custom_attributes (untested). With normal amounts of templates, I don't expect it will be significant either way.

(A 3rd option would be a JSONB column; I don't know enough about that to say anything.)

simon3z · 2017-09-04T08:33:31Z

@cben this is still waiting a technical review of the dependency ManageIQ/manageiq-schema#32

simon3z · 2017-09-04T14:38:43Z

👍
@miq-bot assign blomquisg

simon3z · 2017-09-04T14:39:18Z

@cben can you approve?

miq-bot added the sql migration label Jun 20, 2017

miq-bot added enhancement providers/containers labels Jun 20, 2017

zakiva mentioned this pull request Jun 20, 2017

Container Template: Parse object_labels ManageIQ/manageiq-providers-openshift#25

Merged

zakiva force-pushed the add_template_labels branch from b96f788 to 34a0e28 Compare June 20, 2017 14:36

chessbyte assigned simon3z Jun 20, 2017

zakiva force-pushed the add_template_labels branch from 34a0e28 to b3a13e0 Compare June 21, 2017 08:56

miq-bot added the unmergeable label Jun 22, 2017

zakiva mentioned this pull request Jul 5, 2017

Add object_labels to Container Template ManageIQ/manageiq-schema#32

Merged

zakiva force-pushed the add_template_labels branch from b3a13e0 to 32957eb Compare July 5, 2017 08:33

miq-bot removed the unmergeable label Jul 5, 2017

lfu reviewed Jul 5, 2017

View reviewed changes

lfu reviewed Jul 6, 2017

View reviewed changes

zakiva mentioned this pull request Jul 18, 2017

Add Container Template Spec ManageIQ/manageiq-providers-openshift#37

Merged

miq-bot removed the sql migration label Jul 18, 2017

miq-bot added the unmergeable label Aug 2, 2017

Container Template: Add object_labels

c00ba9e

zakiva force-pushed the add_template_labels branch from 32957eb to c00ba9e Compare August 21, 2017 12:56

miq-bot removed the unmergeable label Aug 21, 2017

zakiva mentioned this pull request Aug 22, 2017

Container Template: Allow instantiation with object_labels ManageIQ/manageiq-providers-kubernetes#98

Merged

zeari approved these changes Sep 4, 2017

View reviewed changes

miq-bot assigned blomquisg and unassigned simon3z Sep 4, 2017

cben approved these changes Sep 5, 2017

View reviewed changes

Fryguy merged commit c44efe3 into ManageIQ:master Sep 6, 2017

Fryguy added this to the Sprint 69 Ending Sep 18, 2017 milestone Sep 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Container Template: Add object_labels #15406

Container Template: Add object_labels #15406

zakiva commented Jun 20, 2017 •

edited

Loading

zakiva commented Jun 20, 2017

zakiva commented Jun 20, 2017

simon3z commented Jun 20, 2017

zakiva commented Jun 21, 2017

simon3z commented Jun 21, 2017

miq-bot commented Jun 22, 2017

Fryguy commented Jun 22, 2017

lfu commented Jun 22, 2017

simon3z commented Jul 4, 2017

zakiva commented Jul 5, 2017

lfu Jul 5, 2017

zakiva Jul 5, 2017

lfu Jul 6, 2017

lfu commented Jul 5, 2017

zakiva commented Jul 5, 2017 •

edited

Loading

lfu left a comment

zakiva commented Jul 18, 2017

zakiva commented Jul 18, 2017

miq-bot commented Aug 2, 2017

miq-bot commented Aug 21, 2017

cben commented Sep 3, 2017

simon3z commented Sep 4, 2017

simon3z commented Sep 4, 2017

simon3z commented Sep 4, 2017

Container Template: Add object_labels #15406

Container Template: Add object_labels #15406

Conversation

zakiva commented Jun 20, 2017 • edited Loading

zakiva commented Jun 20, 2017

zakiva commented Jun 20, 2017

simon3z commented Jun 20, 2017

zakiva commented Jun 21, 2017

simon3z commented Jun 21, 2017

miq-bot commented Jun 22, 2017

Fryguy commented Jun 22, 2017

lfu commented Jun 22, 2017

simon3z commented Jul 4, 2017

zakiva commented Jul 5, 2017

lfu Jul 5, 2017

Choose a reason for hiding this comment

zakiva Jul 5, 2017

Choose a reason for hiding this comment

lfu Jul 6, 2017

Choose a reason for hiding this comment

lfu commented Jul 5, 2017

zakiva commented Jul 5, 2017 • edited Loading

lfu left a comment

Choose a reason for hiding this comment

zakiva commented Jul 18, 2017

zakiva commented Jul 18, 2017

miq-bot commented Aug 2, 2017

miq-bot commented Aug 21, 2017

cben commented Sep 3, 2017

simon3z commented Sep 4, 2017

simon3z commented Sep 4, 2017

simon3z commented Sep 4, 2017

zakiva commented Jun 20, 2017 •

edited

Loading

zakiva commented Jul 5, 2017 •

edited

Loading