implement service level labels #82

concaf · 2017-03-27T15:37:29Z

Fixes #23

surajssd · 2017-04-04T11:46:33Z

pkg/object/object.go

@@ -31,6 +33,7 @@ type Container struct {
 type Service struct {
 	Name       string
 	Containers []Container
+	Labels     Label


in object.go all the types are made of builtin types as opposed to ones like this! I think @tnozicka can shade more light on it!

What I mean here is that do Labels map[string]string as opposed to Labels Label

I think this is correct.

surajssd · 2017-04-04T11:53:07Z

pkg/object/object.go

@@ -22,6 +22,8 @@ type EnvVariable struct {
 	Value string
 }

+type Label map[string]string


Move this definition in encoding.go

This should be here, that is correct.
But just rename it to Labels, so it reflects that this holds multiple labels.

surajssd · 2017-04-04T11:59:52Z

pkg/transform/kubernetes/kubernetes.go

+			case *api_v1.Service:
+				transformerUtil.ApplyOpenComposeLabelsToProvider(&service.Labels, &t.ObjectMeta.Labels)
+			case *ext_v1beta1.Deployment:
+				transformerUtil.ApplyOpenComposeLabelsToProvider(&service.Labels, &t.ObjectMeta.Labels)


I don't know a better way of doing this, the only problem with this method is that it causes problems like we had in kompose. If new k8s object generation support is added and if label application is not done here for that object we now have inconsistent objects generated, which is again hard to test.

If there is an alternate generic method to do this I would love to see that, which wil save us from mistakes in future.

@tnozicka @kadel can suggest a better way of doing it?

Even if we need to do this, we need to design it in such a way that every k8s object should satisfy some form of interface that applies labels.

So that it is enforced and we won't have any inconsistencies.

I think that adding lables to objects should be handled in functions that are responsible for creating those objects. eg CreateServices and CreateDeployments

moved to Create* methods

kadel · 2017-04-05T12:17:39Z

pkg/object/object.go

@@ -22,6 +22,8 @@ type EnvVariable struct {
 	Value string
 }

+type Label map[string]string


This should be here, that is correct.
But just rename it to Labels, so it reflects that this holds multiple labels.

kadel · 2017-04-05T12:18:14Z

pkg/object/object.go

@@ -31,6 +33,7 @@ type Container struct {
 type Service struct {
 	Name       string
 	Containers []Container
+	Labels     Label


I think this is correct.

kadel · 2017-04-05T12:19:26Z

pkg/encoding/v1/encoding.go

@@ -151,6 +151,19 @@ func (raw *EnvVariable) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	return nil
 }

+type Label map[string]string


this should reference Label type from object package.
type Label object.Label

kadel · 2017-04-06T12:31:58Z

Don't forget about labels in PodTemplateSpec inside Deployment. This label should be propagated down to Pod level.

surajssd · 2017-04-11T11:44:19Z

for validating if the label value is right you can use the validation function IsValidLabelValue, use this because there is a doc that specifies what qualifies as a label and what does not.

concaf · 2017-04-13T10:22:07Z

@surajssd thanks for pointing that out 👍

I have added the validation, validation tests, tests in encoding_test.go, PTAL!
@surajssd @kadel

kadel

two small changes, otherwise lgtm

kadel · 2017-04-13T13:21:57Z

pkg/transform/kubernetes/kubernetes.go

-					Labels: map[string]string{
-						"service": o.Name,
-					},
+					Labels: *util.MergeMaps(


I would swap order of parameters here.

*util.MergeMaps( &serviceLabels, &map[string]string{"service": o.Name} ),

Map with "service" label should be the last as it should never be overwritten.
Its used as selector for Service and other stuff.

I would also add comment explaining that it has to be last and why is that.

Same thing in other places setting Labels

Totally missed this somehow, fixed now. Thanks for pointing it out.

kadel · 2017-04-13T13:27:18Z

pkg/encoding/v1/encoding_test.go

@@ -319,6 +319,35 @@ containers:
 				},
 			},
 		},
+
+		{
+			"Providing valid label strings",


You are testing whole service here.

You should add test testing (lb *Labels) UnmarshalYAML something like TestLabels_UnmarshalYAML similar to TestEnvVariable_UnmarshalYAML and others.

surajssd · 2017-04-19T16:15:59Z

pkg/encoding/v1/encoding_test.go

+		},
+	}
+
+		for _, tt := range tests {


gofmt error?

yep, fixed, thanks

surajssd · 2017-05-05T07:09:16Z

pkg/util/util.go

+
+// MergeMaps will merge the given maps, but it does not check for conflicts.
+// In case of conflicting keys, the map that is provided later wins.
+// TODO: add to docs about use with caution bits


where do you wanna add those docs to?

to the developer docs, once and when we have it :)

surajssd · 2017-05-05T07:12:39Z

pkg/util/util.go

@@ -69,3 +69,16 @@ func FetchURLWithRetries(url string, attempts int, duration time.Duration) ([]by

 	return data, err
 }
+
+// MergeMaps will merge the given maps, but it does not check for conflicts.
+// In case of conflicting keys, the map that is provided later wins.


nit: how about In case of conflicting keys, the map that is provided later overrides the previous one ?

surajssd

Add docs about the support of this in the file ref. Also add an example of this in examples folder.

surajssd · 2017-05-05T08:57:57Z

Functionality wise this works for me, @containscafeine apart from the changes requested rest LGTM

kadel · 2017-05-05T10:17:04Z

Add docs about the support of this in the file ref. Also add an example of this in examples folder.

Just small note. All files in examples folder should work and do something meaningful.

concaf · 2017-05-08T05:55:08Z

rebase
add docs
add example

@kadel @surajssd fixed comments, rebased, added docs, an example, PTAL!

surajssd · 2017-05-15T15:52:55Z

cool this LGTM

concaf mentioned this pull request Mar 27, 2017

Labels support #23

Closed

surajssd reviewed Apr 4, 2017

View reviewed changes

kadel reviewed Apr 5, 2017

View reviewed changes

concaf force-pushed the add_label_support branch from 48f93ae to 33ca615 Compare April 6, 2017 07:54

concaf force-pushed the add_label_support branch from 33ca615 to 45c4192 Compare April 7, 2017 12:10

concaf force-pushed the add_label_support branch 2 times, most recently from 0e5a26e to dcf2d20 Compare April 13, 2017 10:20

concaf changed the title ~~[WIP] implement service level labels~~ implement service level labels Apr 13, 2017

kadel reviewed Apr 13, 2017

View reviewed changes

concaf force-pushed the add_label_support branch from 998161c to 8f648b7 Compare April 18, 2017 08:54

surajssd reviewed Apr 19, 2017

View reviewed changes

concaf force-pushed the add_label_support branch 2 times, most recently from bf705a7 to fd4abc1 Compare May 2, 2017 14:34

surajssd reviewed May 5, 2017

View reviewed changes

surajssd suggested changes May 5, 2017

View reviewed changes

concaf force-pushed the add_label_support branch from fd4abc1 to 0c8abf9 Compare May 10, 2017 17:27

implement labels

8fb1788

concaf force-pushed the add_label_support branch from 0c8abf9 to 8fb1788 Compare May 15, 2017 14:39

surajssd approved these changes May 15, 2017

View reviewed changes

kadel merged commit 35a7ac7 into redhat-developer:master May 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement service level labels #82

implement service level labels #82

concaf commented Mar 27, 2017

surajssd Apr 4, 2017 •

edited

Loading

kadel Apr 5, 2017

surajssd Apr 4, 2017

kadel Apr 5, 2017

concaf Apr 5, 2017

surajssd Apr 4, 2017 •

edited

Loading

surajssd Apr 4, 2017 •

edited

Loading

kadel Apr 5, 2017

concaf Apr 6, 2017

kadel Apr 5, 2017

kadel Apr 5, 2017

kadel Apr 5, 2017

concaf Apr 5, 2017

kadel commented Apr 6, 2017

surajssd commented Apr 11, 2017

concaf commented Apr 13, 2017

kadel left a comment

kadel Apr 13, 2017

concaf Apr 18, 2017

kadel Apr 13, 2017

concaf Apr 18, 2017

surajssd Apr 19, 2017

concaf Apr 26, 2017

surajssd May 5, 2017

concaf May 15, 2017

surajssd May 5, 2017

concaf May 15, 2017

surajssd left a comment

surajssd commented May 5, 2017

kadel commented May 5, 2017

concaf commented May 8, 2017 •

edited

Loading

surajssd commented May 15, 2017

implement service level labels #82

implement service level labels #82

Conversation

concaf commented Mar 27, 2017

surajssd Apr 4, 2017 • edited Loading

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

surajssd Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

surajssd Apr 4, 2017 • edited Loading

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

kadel commented Apr 6, 2017

surajssd commented Apr 11, 2017

concaf commented Apr 13, 2017

kadel 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

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

surajssd left a comment

Choose a reason for hiding this comment

surajssd commented May 5, 2017

kadel commented May 5, 2017

concaf commented May 8, 2017 • edited Loading

surajssd commented May 15, 2017

surajssd Apr 4, 2017 •

edited

Loading

surajssd Apr 4, 2017 •

edited

Loading

surajssd Apr 4, 2017 •

edited

Loading

concaf commented May 8, 2017 •

edited

Loading