introduce portMappings shortcut #242

concaf · 2017-08-31T12:18:05Z

add docs

This commit adds a shortcut to ServiceSpec called portMappings.
This lets us set the port, targetPort and the protocol for a
service in a single line. This is parsed and converted to a
Kubernetes ServicePort object.

portMappings is an array of port:targetPort/protocol
definitions, so the syntax looks like -

portMappings:

port:targetPort/protocol
port:targetPort/protocol

Also, tests are added to test the added behavior.

The logic of auto populating names for the services has been moved
from fix.go to populators.go, because now the auto population
happens once the []ServicePort is populated from the both the
inputs, ServiceSpecMod.Ports and ServiceSpecMod.PortMappings

Fixes #216

kadel · 2017-09-04T12:08:55Z

docs/file-reference.md

@@ -30,6 +30,8 @@ services:
  - port: 8080
    targetPort: 80
    endpoint: minikube.external/foo
+  portMappings:


this doesn't make sense in this example, as there is nothing running on port 8081

I don't know if this makes sense, why would you expose the same port twice?

I think that all our examples like this have to work and make sense.

I don't think that showing one example with all Kedge keys has any benefit.

I agree, but I think we should cover all the fields in file-reference.md so we can show them in use. Where do you suggest we add that?
I think we should have an example which shows all the fields added/modified by kedge in file-reference, it does not have to be a working example, but something that has all the fields and acts as a quick reference on how and where to use that field.

As for this, should I remove this?

I think that if we show an example with all the fields it will be just hard to read and it won't provide any benefits.

I think that far better is to have multiple examples highlighting given shortcut in the scenario that is best for it. We should show how it should be used, in the scenarios we think is the best for given shortcut and highlight our intent behind it.

Having multiple examples in files-reference.md will be also mess, so we can just reference examples in docs/examples/

In this case, we can just link to docs/examples/portMappings/httpd.yaml with some short explanation.

kadel · 2017-09-04T14:33:57Z

docs/file-reference.md

+- <port:targetPort/protocol>
+```
+
+The only field mandatory here is "port".


Can you specify that you are talking about port part in the portMappings. When I read this for the first time I thought you are talking about port in Service.

How about this?
'The only mandatory part of the portMappings is port'

makes sense, updated

kadel · 2017-09-05T08:24:39Z

docs/file-reference.md

@@ -30,6 +30,8 @@ services:
  - port: 8080
    targetPort: 80
    endpoint: minikube.external/foo
+  portMappings:
+  - 8888:80/TCPf


s/TCPf/TCP/

whoops, fixed

kadel · 2017-09-11T14:34:41Z

Needs rebase, but otherwise this PR LGTM

concaf · 2017-09-12T03:40:28Z

@kadel rebased

surajssd · 2017-09-12T12:05:17Z

docs/examples/portMappings/README.md

+  - name: httpd
+    portMappings:
+    - 8080:80/TCP
+```


Can you also mention under known issues or some kinda note saying if you are using portMappings you cannot use the shortcut expose for creating ingress ?

Is that a known issue? endpoint shortcut is only supported with ports: field, and portMappings: is at the same level and not nested under ports. It's not something that's not supported, it's something that's not part of the syntax of portMappings.

I just thought it would be better if we be explicit about it.

surajssd · 2017-09-12T12:05:44Z

docs/examples/portMappings/httpd.yaml

+containers:
+- image: centos/httpd
+services:
+  - name: httpd


nit: extra indentation in this section!

surajssd · 2017-09-12T12:11:14Z

pkg/spec/populators.go

 	api_v1 "k8s.io/client-go/pkg/api/v1"
+	"strconv"


can you arrange the imports similar to the comment here #149 (comment)

surajssd · 2017-09-12T12:14:22Z

pkg/spec/populators.go

@@ -150,6 +167,93 @@ func populateEnvFrom(c Container, cms []ConfigMapMod, secrets []SecretMod) (Cont
 	return c, nil
 }

+// Parse the string the get the port, targetPort and protocol
+// information, and then return the resulting ServicePort object
+func parsePortMapping(pm string) (*api_v1.ServicePort, error) {


the placement of this function is wrong can, you place it in resources.go or util.go ? Because this is file of all the populators !

well, this is parsing the port mapping and populating a ServicePort. What do we really mean by a populator? Will help in moving this around.

this function is just parsing, and not populating so logically it should reside somewhere but not with all other populatiors.

ack, moved to resources.go

surajssd · 2017-09-12T12:23:14Z

pkg/spec/populators_test.go

+	}
+}
+
+func TestParsePortMapping(t *testing.T) {


the placement of this function is wrong can, you place it in resources.go or util.go ? Because this is file of all the populators !

Since the code will move so should the tests!

surajssd · 2017-09-12T12:28:07Z

pkg/spec/populators.go

+			// Only populate if the port name is not already specified
+			if len(servicePorts[i].Name) == 0 {
+				servicePorts[i].Name = serviceName + "-" + strconv.FormatInt(int64(servicePorts[i].Port), 10)
+				fmt.Println(servicePorts[i].Name)


Also remove this debug statement!

whoops, removed

surajssd · 2017-09-12T12:30:17Z

pkg/spec/populators_test.go

@@ -21,6 +21,9 @@ import (
 	"sort"
 	"testing"

+	"fmt"


can you arrange the imports similar to the comment here #149 (comment)

surajssd · 2017-09-12T12:35:27Z

pkg/spec/populators_test.go

+			switch test.success {
+			case true:
+				if err != nil {
+					t.Errorf("Expected test to pass but got an error -\n%v", err)


this is test failure situtation and you are in a function of a test, so use t.Fatal.

I was actually confused why are doing a t.Fatalf for rest of the tests.

t.Error marks the function as having failed but continues execution. (FWIW, 2,611 code results in kubernetes/kubernetes)

t.Fatal marks the function as having failed and stops its execution. (FWIW, 596 code results in kubernetes/kubernetes)

There are other checks which should continue to happen in the function if one test fails so the resulting test report is more comprehensive.

Each test here runs in separate function when you do t.Run() so you have to do t.Fatal() or else t.Error() and return.

There are other checks which should continue to happen in the function if one test fails so the resulting test report is more comprehensive.

One test fail won't affect other tests, they will run as usual, since each test is running in its own test function.

No, t.Errorf does not require to return.

One test fail won't affect other tests, they will run as usual, since each test is running in its own test function.

It does not affect other tests, but the further checks for the same test do not happen when t.Fatal is used. This cuts down the test reports and does not give a full report of failing tests.

What is the guarantee that the results we get after a failing condition are correct ?

This is not about a guarantee, this is about getting the most extensive test report to troubleshoot the test failures. It does not make sense to fix one test, and then run tests again to find out that another one is failing, and so on.
It's also the used convention, rightfully so, in Kubernetes as well.

surajssd · 2017-09-12T12:37:36Z

pkg/spec/populators_test.go

+				}
+			}
+
+			if !reflect.DeepEqual(sp, test.servicePort) {


we need this check only when tests pass which is case true

For a general testing pattern, if we add a case where we need to test the function output even when an error occurs, we will miss that if we check for this inside case: true.
e.g.

{ name: "something", portMapping: "1337:1338::1339/TCP", servicePort: &api_v1.ServicePort{}, success: false, },

I think we should keep it outside.

We can make changes when we need it like that, right now we don't!

unsure what we lose by keeping it this way

It good to add things when we need them!

surajssd · 2017-09-12T12:37:49Z

pkg/spec/populators_test.go

+				}
+			case false:
+				if err == nil {
+					t.Errorf("Expected %v to fail, but test passed!", test.portMapping)


this is test failure situtation and you are in a function of a test, so use t.Fatal.

surajssd · 2017-09-12T12:52:55Z

Since this is a feature, can you add a e2e test which has two ports specified and the test checks if both of them are serving ?

This commit adds a shortcut to ServiceSpec called portMappings. This lets us set the port, targetPort and the protocol for a service in a single line. This is parsed and converted to a Kubernetes ServicePort object. portMappings is an array of `port:targetPort/protocol` definitions, so the syntax looks like - portMappings: - <port:targetPort/protocol> - <port:targetPort/protocol> Also, tests are added to test the added behavior. The logic of auto populating names for the services has been moved from fix.go to populators.go, because now the auto population happens once the []ServicePort is populated from the both the inputs, ServiceSpecMod.Ports and ServiceSpecMod.PortMappings Fixes kedgeproject#216

surajssd · 2017-09-13T09:51:11Z

pkg/spec/resources_test.go

+			}
+
+			if !reflect.DeepEqual(sp, test.servicePort) {
+				t.Errorf("Expected ServicePort to be -\n%v\nBut got -\n%v", spew.Sprint(test.servicePort), spew.Sprint(sp))


Forked the discussion from #242 (comment)

Consider a case if the test is expected to pass, but it fails then the test should not go any further than

t.Errorf("Expected test to pass but got an error -\n%v", err)

which is line #3315

because we are sure that it will surely throw error at line #324 which is

t.Errorf("Expected ServicePort to be -\n%v\nBut got -\n%v", spew.Sprint(test.servicePort), spew.Sprint(sp))

because the variable sp is nil because of failed test. So if we know further in path if a test is surely gonna fail, I don't think there is point in continuing atleast in this case. Because that won't be comprehensive test output but useless verbose output on a sure thing that is always gonna happen.

For e.g. consider:

$ go test -v github.com/kedgeproject/kedge/pkg/spec -run TestParsePortMapping === RUN TestParsePortMapping === RUN TestParsePortMapping/Nothing_is_passed,_not_even_port === RUN TestParsePortMapping/Only_'port'_is_passed === RUN TestParsePortMapping/port:targetPort_is_passed === RUN TestParsePortMapping/port/protocol_is_passed === RUN TestParsePortMapping/port:targetPort/protocol_is_passed === RUN TestParsePortMapping/Invalid_protocol_(neither_TCP_nor_UDP)_is_passed === RUN TestParsePortMapping/Multiple_protocols_passed,_multiple_'/'_test === RUN TestParsePortMapping/Non_int_port_is_passed === RUN TestParsePortMapping/Non_int_targetPort_is_passed === RUN TestParsePortMapping/More_than_2_ports_passed,_multiple_':'_test --- FAIL: TestParsePortMapping (0.00s) --- PASS: TestParsePortMapping/Nothing_is_passed,_not_even_port (0.00s) --- PASS: TestParsePortMapping/Only_'port'_is_passed (0.00s) --- PASS: TestParsePortMapping/port:targetPort_is_passed (0.00s) --- PASS: TestParsePortMapping/port/protocol_is_passed (0.00s) --- FAIL: TestParsePortMapping/port:targetPort/protocol_is_passed (0.00s) resources_test.go:315: Expected test to pass but got an error - port is not an int: strconv.ParseInt: parsing "": invalid syntax resources_test.go:324: Expected ServicePort to be - <*>&ServicePort{Name:,Protocol:UDP,Port:1337,TargetPort:1338,NodePort:0,} But got - <nil> --- PASS: TestParsePortMapping/Invalid_protocol_(neither_TCP_nor_UDP)_is_passed (0.00s) --- PASS: TestParsePortMapping/Multiple_protocols_passed,_multiple_'/'_test (0.00s) --- PASS: TestParsePortMapping/Non_int_port_is_passed (0.00s) --- PASS: TestParsePortMapping/Non_int_targetPort_is_passed (0.00s) --- PASS: TestParsePortMapping/More_than_2_ports_passed,_multiple_':'_test (0.00s) FAIL exit status 1 FAIL github.com/kedgeproject/kedge/pkg/spec 0.044s

So for failed test only this is sufficient

resources_test.go:315: Expected test to pass but got an error - port is not an int: strconv.ParseInt: parsing "": invalid syntax

We don't need to have the next set of lines:

resources_test.go:324: Expected ServicePort to be - <*>&ServicePort{Name:,Protocol:UDP,Port:1337,TargetPort:1338,NodePort:0,} But got - <nil>

A function like the following is being tested -

func a() { behavior x behavior y behavior z }

We write a test -

func TestA() { t.Run("test a()", func(t *testing.T) { t.Fatal("fail if behavior x fails") t.Fatal("fail if behavior y fails") t.Fatal("fail if behavior z fails") } }

Now, when behavior x fails, behavior y and behavior z will not be tested, in our test report we will only get failure for behavior x. When we fix that, we will then get an error for behavior y, then after fixing that we get the report for behavior z.

OTOH, if we use -

func TestA() { t.Run("test a()", func(t *testing.T) { t.Error("fail if behavior x fails") t.Error("fail if behavior y fails") t.Error("fail if behavior z fails") } }

In this case we will get test failure reports for all the behaviors x, y and z at the same time and we can go ahead and do the fixes simultaneously.

The point of a test report is to find all the failures happening in the tests and making the report as extensive as we can so we can get what's breaking what, instead not to find failures in an iterative manner.

surajssd · 2017-09-13T09:51:54Z

docs/examples/portMappings/README.md

+services:
+  - name: httpd
+    portMappings:
+    - 8080:80/TCP


Similar to #242 (comment)

surajssd · 2017-09-13T09:52:40Z

pkg/spec/populators.go

 	api_v1 "k8s.io/client-go/pkg/api/v1"
+
+	"github.com/pkg/errors"


can you arrange the imports similar to the comment here #149 (comment)

There are no kedge imports here, these are in the order of stdlib first, then third party. What are you suggesting?

surajssd · 2017-09-13T09:53:09Z

pkg/spec/populators_test.go

 	"reflect"
 	"sort"
 	"testing"

 	api_v1 "k8s.io/client-go/pkg/api/v1"
+


nit: s/\n//

can you arrange the imports similar to the comment here #149 (comment)

This is similar to what's done here - https://github.com/kedgeproject/kedge/blob/master/pkg/spec/resources.go#L19-L38 - what's wrong?

surajssd

Since this is a feature, can you add a e2e test which has two ports specified and the test checks if both of them are serving ?

concaf added the status/work-in-progress label Aug 31, 2017

concaf force-pushed the portmapping_shortcut branch 3 times, most recently from 560f32b to cb2297f Compare September 1, 2017 08:50

concaf added status/needs-review and removed status/work-in-progress labels Sep 1, 2017

kadel reviewed Sep 4, 2017

View reviewed changes

kadel reviewed Sep 5, 2017

View reviewed changes

concaf force-pushed the portmapping_shortcut branch from ce16871 to 4b2f2ab Compare September 5, 2017 08:39

pradeepto requested a review from surajssd September 10, 2017 07:32

surajssd added the status/needs-rebase label Sep 11, 2017

concaf force-pushed the portmapping_shortcut branch from 7701726 to b888cb3 Compare September 12, 2017 03:39

concaf removed status/needs-rebase status/needs-review labels Sep 12, 2017

kadel approved these changes Sep 12, 2017

View reviewed changes

surajssd suggested changes Sep 12, 2017

View reviewed changes

surajssd reviewed Sep 12, 2017

View reviewed changes

surajssd suggested changes Sep 12, 2017

View reviewed changes

concaf added 3 commits September 13, 2017 14:16

changes in example and populators

9facce1

move parsePortMapping() to resources.go

304c6f4

concaf force-pushed the portmapping_shortcut branch from 84efe7f to 304c6f4 Compare September 13, 2017 08:48

surajssd suggested changes Sep 13, 2017

View reviewed changes

surajssd mentioned this pull request Sep 13, 2017

docs: define go import convention #258

Merged

doc fixes, add e2e tests

c8c9181

surajssd merged commit 38b9d03 into kedgeproject:master Sep 14, 2017

		api_v1 "k8s.io/client-go/pkg/api/v1"

		"github.com/pkg/errors"

introduce portMappings shortcut #242

introduce portMappings shortcut #242

Conversation

concaf commented Aug 31, 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

kadel commented Sep 11, 2017

concaf commented Sep 12, 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

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

surajssd commented Sep 12, 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

surajssd left a comment

Choose a reason for hiding this comment

concaf commented Aug 31, 2017 •

edited

Loading