Added support for providing replicas #66

surajssd · 2017-03-10T10:20:19Z

A user can now provide the number of replicas of the
service that are needed, using a field called replicas
in opencompose service section.

This is an optional field, if nothing is provided then
the value defaults to 1.

Fixes #19

kadel · 2017-03-10T10:48:39Z

Commented in the original issue #19 (comment)

tnozicka

Allow specifying replica count for service #19 (comment)
missing unit tests

tnozicka · 2017-03-10T19:16:47Z

@surajssd FYI and to save you time, you need to move from int32 to *int32.

tnozicka · 2017-03-10T19:19:24Z

pkg/cmd/convert.go

@@ -75,7 +75,7 @@ func GetValidatedObject(v *viper.Viper, cmd *cobra.Command, out, outerr io.Write

 		o, err := decoder.Decode(data)
 		if err != nil {
-			return nil, fmt.Errorf("could not unmarsha data for file '%s': %s", file, err)
+			return nil, fmt.Errorf("could not unmarshal data for file '%s': %s", file, err)


Could you please make it a different commit as it has no relation to Added support for providing replicas?

surajssd · 2017-03-13T10:33:17Z

pkg/transform/kubernetes/kubernetes.go

+	// Set replica value only if it is given
+	if o.Replicas > 0 {
+		replicas := o.Replicas
+		d.Spec.Replicas = &replicas


This is a better thing to do or should I do: d.Spec.Replicas = &o.Replicas ?
Thoughts: @tnozicka @containscafeine ?

@surajssd d.Spec.Replicas = &o.Replicas should be fine, no?

@containscafeine i thought it will have to save entire object o to just assign value, until d is alive. But if I can copy it in some variable then it will be less wastage of resources?

@surajssd d.Spec.Replicas = o.Replicas you should make o.Replicas pointer as well because it will be a pointer from the parser too

oh yes, makes sense

surajssd · 2017-03-13T10:36:57Z

pkg/encoding/v1/encoding.go

@@ -319,6 +320,9 @@ func (d *Decoder) Decode(data []byte) (*object.OpenCompose, error) {
 			Name: string(s.Name),
 		}

+		// user given value of replicas
+		os.Replicas = s.Replicas


using values as it is here and checking down whether it should land in deployment or not? thoughts: @tnozicka

probably out-of date now

surajssd · 2017-03-13T10:38:26Z

pkg/encoding/v1/encoding_test.go

+								Image: "tomaskral/nonroot-nginx",
+							},
+						},
+						Replicas: 0,


since using values as it is so not much changes, checks done at a level up when creating deployment, so original info is not lost from opencompose as well as in cluster.

tnozicka · 2017-03-13T11:02:29Z

pkg/encoding/v1/encoding.go

@@ -184,6 +184,7 @@ func (c *Container) UnmarshalYAML(unmarshal func(interface{}) error) error {
 type Service struct {
 	Name       ResourceName `yaml:"name"`
 	Containers []Container  `yaml:"containers"`
+	Replicas   int32        `yaml:"replicas,omitempty"`


it has to be a pointer here otherwise you can't distinguish between user setting 0 and not specifying the value

tnozicka · 2017-03-14T15:29:37Z

pkg/encoding/v1/encoding.go

@@ -319,6 +320,9 @@ func (d *Decoder) Decode(data []byte) (*object.OpenCompose, error) {
 			Name: string(s.Name),
 		}

+		// user given value of replicas
+		os.Replicas = s.Replicas


probably out-of date now

tnozicka · 2017-03-14T15:35:40Z

pkg/transform/kubernetes/kubernetes.go

@@ -106,6 +106,11 @@ func (t *Transformer) CreateDeployments(o *object.Service) ([]runtime.Object, er
 		},
 	}

+	// Set replica value only if it is given
+	if o.Replicas != nil {
+		d.Spec.Replicas = o.Replicas


only L111 is needed since it's now a pointer

tnozicka · 2017-03-14T15:50:12Z

pkg/encoding/v1/encoding_test.go

@@ -419,5 +419,44 @@ volumes: []
 			continue
 		}
 	}
+}


I don't like these unit test. We do table driven testing (a bit of Golang way) everywhere else.

There should be 2 unit tests for:

all the test cases (nil, 0, 42, fail, ...) - TestService_UnmarshalYAML (you can get inspiration here)
The unit test for service hasn't been there before which is my fault, but If you wouldn't mind adding it there, that will be great

to ensure the value gets copied (probably just add one case to existing test) -https://github.com/surajssd/opencompose/blob/8e34ab28588e02b85fada5cc52661cf300f36b0a/pkg/encoding/v1/encoding_test.go#L147

tnozicka · 2017-03-14T15:51:24Z

pkg/encoding/v1/encoding.go

@@ -319,6 +320,9 @@ func (d *Decoder) Decode(data []byte) (*object.OpenCompose, error) {
 			Name: string(s.Name),
 		}

+		// user given value of replicas


it's now a pointer to the value; maybe just drop the comment since its obvious anyway

surajssd · 2017-03-15T15:31:53Z

pkg/encoding/v1/encoding_test.go

+version: 0.1-dev
+services:
+- name: helloworld
+  replicas: -1


even after giving value as negative this test passes, since the check on negativity happens later? Apart from adding tests about createdeployment and verifying this actually fails @tnozicka do you have any thought on handling this well?

parser should check this

surajssd · 2017-03-15T15:32:47Z

pkg/transform/kubernetes/kubernetes.go

+		} else {
+			return nil, fmt.Errorf("Replica count is negative in service: %q", o.Name)
+		}
+	}


this is where I have added the check and this is needed otherwise the artifact is generated with negative number and k8s won't take that!

actually, parser should be the one checking that since it's invalid input

Added checks to parser as well

If this is checked in parser, does it make sense to check it here again?

@kadel agreed. That's what my previous comment was suppose to mean (move, not copy the check) - this isn't task transformer should do

If I remove check from here, how do we address the condition where someone wants to use the opencompose as lib?

and we wanna do the validation? ideally for an app like this where data is coming from multiple places we should have a layer which does it for us? WDYT @tnozicka ?

May be we could use object.Validate for this.

All validation logic should be in one place

this makes sense to me!

tnozicka · 2017-03-15T15:36:01Z

pkg/transform/kubernetes/kubernetes.go

+		} else {
+			return nil, fmt.Errorf("Replica count is negative in service: %q", o.Name)
+		}
+	}


actually, parser should be the one checking that since it's invalid input

tnozicka · 2017-03-15T15:38:48Z

pkg/encoding/v1/encoding_test.go

+	}
+
+	for _, test := range tests {
+		t.Logf("Running test: %q", test.Name)


please use t.Run where you can pass the name

we are moving to using subtests (https://blog.golang.org/subtests)

https://github.com/tnozicka/opencompose/blob/a21430158830bff240a8252aa4b609e994907238/pkg/encoding/v1/encoding_test.go#L207

tnozicka · 2017-03-15T15:40:21Z

pkg/encoding/v1/encoding_test.go

+version: 0.1-dev
+services:
+- name: helloworld
+  replicas: -1


parser should check this

tnozicka · 2017-03-15T15:41:18Z

pkg/encoding/v1/encoding_test.go

+			"Giving replica as string to see parsing fails",
+			false, `
+name: frontend
+replicas: notint


probably a typo: s/notint/nonint/ or s/notint/non-int/

It's just for showing this is 'not an integer' value

kadel · 2017-03-20T12:29:56Z

Can you please update docs (file-reference) as part of this PR?

surajssd · 2017-03-21T10:12:19Z

@kadel done!

kadel · 2017-03-22T12:30:45Z

docs/file-reference.md

+| int32 |    no    |
+
+Number of desired pods of this particluar service. This is an optional field.
+If nothing is provided the artifact generated won't have anything either.


I don't know about this sentence :-(
It won't say anything useful for people that are not familiar with Kubernetes.
I think that we should avoid mentioning generated artifacts as much as possible.

Can we just delete that last sentence sentence?

tnozicka · 2017-03-22T14:48:48Z

docs/file-reference.md

@@ -38,11 +39,12 @@ Version of OpenCompose format.
 Describes one service. Service can be composed of one or multiple containers which are scheduled 
 together and can communicate using localhost. (Containers in the same service share network namespace.)

-Each service has name and list of the containers.
+Each service has name, replicas and list of the containers.


it doesn't have to have replicas

surajssd · 2017-03-30T09:21:07Z

@kadel @tnozicka updated as you guys mentioned, will squash when you guys give LGTM!

kadel · 2017-03-30T09:29:34Z

pkg/object/object.go

+	// validate containers
+
+	// validate replicas
+	if err := validateReplicas(s.Replicas); err != nil {


Stupid question as usual 😄
If replica check is just one line, and it is used in one place, does it make sense to have a separate function for it? Wouldn't be code more readable if actual check was here? 😇

I wanted to make it testable so separated it out!

than you testing separately one if statement?
I don't know, but for me it looks like it is just overcomplicating things

Validate can be tested as whole.

Even I thought this might be overdoing but then there arent other tests so just for testing replicas I will need to create entire stucts object.

And here I can do specific tests like these one: https://github.com/redhat-developer/opencompose/pull/66/files#diff-4f9edbaf8553598b5a269f232bab233cR16

As soon as you add another validation test to this you will have to test whole Validate function anyway.

updated as per comments!

surajssd · 2017-03-30T09:47:42Z

pkg/transform/kubernetes/kubernetes_test.go

+			true,
+			&object.Service{
+				Name:     name,
+				Replicas: goutil.Int32Addr(-1),


@kadel also comment on this test?

surajssd · 2017-03-30T09:48:37Z

pkg/encoding/v1/encoding_test.go

+`,
+			&Service{
+				Name:     "frontend",
+				Replicas: goutil.Int32Addr(-1),


@kadel also this test, please comment if this is needed ?

kadel

lgtm

surajssd · 2017-04-01T06:52:38Z

@kadel is it okay to merge?

kadel · 2017-04-03T12:22:36Z

@kadel is it okay to merge?

lets wait for @tnozicka review

surajssd · 2017-04-04T10:55:08Z

ping @tnozicka

tnozicka · 2017-04-04T12:22:52Z

docs/file-reference.md

+#### replicas
+| type  | required |
+|-------|----------|
+| int32 |    no    |


wouldn't uint32 be more descriptive since any negative value is invalid. Or it would be good to mention it somewhere else

should the internal object be also changed to uint32?

I think int32 in the code is ok.

I'm just thinking if it makes sense in documentation mention int32 type, maybe just integer?

tnozicka · 2017-04-04T12:25:17Z

pkg/encoding/v1/encoding_test.go

+	}
+
+	for _, test := range tests {
+		t.Run(fmt.Sprintf("Running test: %q", test.Name), func(t *testing.T) {


There should be just a test name; go test tool will make the appropriate announcement on what is running

tnozicka · 2017-04-04T12:30:16Z

pkg/encoding/v1/encoding_test.go

+
+	tests := []struct {
+		Name           string
+		ExpectedSucess bool


consistent naming with the rest would be Succeed

tnozicka · 2017-04-04T12:32:52Z

pkg/encoding/v1/encoding_test.go

+			}
+
+			if !test.ExpectedSucess {
+				t.Errorf("Expected %#v to fail, but succeeded!", test.RawService)


you can merge these 2 lines by using t.Fatalf

tnozicka · 2017-04-04T12:35:33Z

pkg/goutil/goutil_test.go

+
+	for _, test := range tests {
+		if test.input != *test.output {
+			t.Errorf("The input and output doesn't match, for %d", test.input)


logically you should print both of those values to know what's wrong, not just input

tnozicka · 2017-04-04T12:39:08Z

pkg/object/object.go

+	// validate containers
+
+	// validate replicas
+	if s.Replicas != nil && *s.Replicas < 0 {


such checks should be done in the parser as I have mentioned in my previous comments. it's n invalid input for the parser and it is the only place we can have more info like a line number - and all the other checks are done in parser as well!

Logically, the value is valid for parser because parser is only supposed to parse data for its type and not validate if it is correct or not. That's the job of validator.

And I would like to have validation happening in only one place not twice. Someone using opencompose as lib might not be passing validated data so how are we gonna validate that?

Letes move on, and go with this as it is.
We can figure out how we are going to do all validations in another PR.

#92

So we can merge this one?

see my other comment regarding docs (https://github.com/redhat-developer/opencompose/pull/66/files#r110106733)
than if you squash it, i think we can merge it

Sure on it now!

tnozicka · 2017-04-04T12:43:24Z

pkg/object/object.go

+
+	// validate replicas
+	if s.Replicas != nil && *s.Replicas < 0 {
+		return fmt.Errorf("Replica count is negative in service: %q", s.Name)


s/is/shall not be/

tnozicka · 2017-04-04T12:44:52Z

pkg/transform/kubernetes/kubernetes_test.go

+
+	tests := []struct {
+		Name           string
+		ExpectSuccess  bool


non-consistent naming as above

tnozicka · 2017-04-04T12:47:44Z

pkg/transform/kubernetes/kubernetes_test.go

+
+	transformer := Transformer{}
+	for _, test := range tests {
+


tnozicka · 2017-04-04T12:48:02Z

pkg/transform/kubernetes/kubernetes_test.go

+	transformer := Transformer{}
+	for _, test := range tests {
+
+		t.Run(fmt.Sprintf("Running test: %q", test.Name), func(t *testing.T) {


there should be just name of the test

A user can now provide the number of replicas of the service that are needed, using a field called `replicas` in opencompose `service` section. This is an optional field, if nothing is provided then the no default value is assumed. Fixes redhat-developer#19

all issues addressed, validation will be fixed in another pr #92

tnozicka self-requested a review March 10, 2017 19:10

tnozicka suggested changes Mar 10, 2017

View reviewed changes

tnozicka reviewed Mar 10, 2017

View reviewed changes

tnozicka added the backwards compatible change label Mar 10, 2017

surajssd force-pushed the replicas branch 3 times, most recently from 50a5235 to 2c6c608 Compare March 13, 2017 10:29

surajssd commented Mar 13, 2017

View reviewed changes

surajssd force-pushed the replicas branch from 2c6c608 to 7efbcad Compare March 13, 2017 10:56

tnozicka self-requested a review March 13, 2017 11:04

tnozicka suggested changes Mar 13, 2017

View reviewed changes

tnozicka assigned surajssd Mar 14, 2017

surajssd force-pushed the replicas branch from 7efbcad to 8e34ab2 Compare March 14, 2017 12:29

tnozicka suggested changes Mar 14, 2017

View reviewed changes

surajssd force-pushed the replicas branch 2 times, most recently from 546162d to df245ff Compare March 15, 2017 15:29

surajssd commented Mar 15, 2017

View reviewed changes

tnozicka suggested changes Mar 15, 2017

View reviewed changes

surajssd force-pushed the replicas branch 5 times, most recently from ba67038 to 4b49e16 Compare March 16, 2017 10:03

surajssd force-pushed the replicas branch from 4b49e16 to fc0c100 Compare March 21, 2017 10:01

kadel reviewed Mar 22, 2017

View reviewed changes

tnozicka reviewed Mar 22, 2017

View reviewed changes

surajssd force-pushed the replicas branch 2 times, most recently from d702237 to b5b5086 Compare March 30, 2017 09:15

surajssd force-pushed the replicas branch from b5b5086 to 947a842 Compare March 30, 2017 09:22

kadel reviewed Mar 30, 2017

View reviewed changes

surajssd commented Mar 30, 2017

View reviewed changes

surajssd force-pushed the replicas branch from 84cf4b1 to a9e32bc Compare March 31, 2017 07:19

kadel approved these changes Mar 31, 2017

View reviewed changes

tnozicka previously requested changes Apr 4, 2017

View reviewed changes

surajssd force-pushed the replicas branch from a9e32bc to 514791c Compare April 5, 2017 10:13

surajssd force-pushed the replicas branch from 514791c to 1543961 Compare April 6, 2017 12:39

kadel approved these changes Apr 6, 2017

View reviewed changes

kadel merged commit 59729ba into redhat-developer:master Apr 6, 2017

Added support for providing replicas #66

Added support for providing replicas #66

Conversation

surajssd commented Mar 10, 2017 • edited Loading

kadel commented Mar 10, 2017

tnozicka left a comment

Choose a reason for hiding this comment

tnozicka commented Mar 10, 2017

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

tnozicka Mar 13, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel Mar 27, 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 Mar 20, 2017

surajssd commented Mar 21, 2017

kadel Mar 22, 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 commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel Mar 30, 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

kadel left a comment

Choose a reason for hiding this comment

surajssd commented Apr 1, 2017

kadel commented Apr 3, 2017

surajssd commented Apr 4, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajssd commented Mar 10, 2017 •

edited

Loading

tnozicka Mar 13, 2017 •

edited

Loading

kadel Mar 27, 2017 •

edited

Loading

kadel Mar 22, 2017 •

edited

Loading

kadel Mar 30, 2017 •

edited

Loading