Added listeners to virtual-router under VirtualService CRD #44
Conversation
kiranmeduri
force-pushed
the
vrouterlistener
branch
from
d92ed48
to
a8de398
Compare
| } | ||
|
|
||
| if len(desired.Listeners) > 0 { | ||
| //there should be only one listener |
There was a problem hiding this comment.
Somewhat tricky logic to read, but it makes sense. I was just considering the options:
+------------------------+---------------------------------+-------------------+
| len(desired.Listeners) | len(target.Data.Spec.Listeners) | routerNeedsUpdate |
+------------------------+---------------------------------+-------------------+
| 0 | 0 | false |
| | | |
| 1 | 0 | true |
| | | |
| 0 | 1 | true |
| | | |
| 1 | 1 | true |
| | | |
| 2 | 1 | true |
+------------------------+---------------------------------+-------------------+
I assume that target.Data.Spec.Listeners contains at most 1 listener?
There was a problem hiding this comment.
yeah I can change this to a loop and assume order is preserved or be defensive and error if > 1. either ways it is still tricky :)
| }{ | ||
| {"virtual-routers are the same", defaultRouter, defaultResult, false}, | ||
| {"virtual-routers has different router name", defaultRouter, resultDifferentRouterName, true}, | ||
| {"virtual-routers has different listener", defaultRouter, resultDifferentRouterListener, true}, |
| CLIENT_PKG=github.com/aws/aws-app-mesh-controller-for-k8s/pkg/client | ||
| APIS_PKG=github.com/aws/aws-app-mesh-controller-for-k8s/pkg/apis | ||
|
|
||
| ${CODEGEN_PKG}/generate-groups.sh all \ | ||
| ${CLIENT_PKG} \ | ||
| ${APIS_PKG} \ | ||
| appmesh:v1beta1 \ | ||
| --go-header-file ${SCRIPT_ROOT}/hack/custom-boilerplate.go.txt | ||
| --output-base "${SCRIPT_ROOT}/../../.." \ |
There was a problem hiding this comment.
Tried this with my setup. I have the project root outside of my GOPATH (allowed with go modules), but in order to make this script work I have aws-app-mesh-controller-for-k8s symlinked into my GOPATH. When I need to run make code-gen I cd into the GOPATH symlink and run it, and the references work. On your branch, I get:
$ make code-gen
./scripts/update-codegen.sh
Generating deepcopy funcs
F0624 23:22:29.046208 47058 main.go:82] Error: Failed executing generator: some packages had errors:
errors in package "github.com/aws/aws-app-mesh-controller-for-k8s/pkg/apis/appmesh/v1beta1":
open ../../../github.com/aws/aws-app-mesh-controller-for-k8s/pkg/apis/appmesh/v1beta1/zz_generated.deepcopy.go: no such file or directory
make: *** [code-gen] Error 255
Are you running this from inside or outside of your GOPATH?
I see that there is a bug with the script as it was, I forgot to rename hack to scripts in the header file, but I'm not sure how you have it set up so this works as is, with --output-base set.
| meshName: color-mesh | ||
| meshName: ${MESH_NAME} | ||
| virtualRouter: | ||
| listeners: |
There was a problem hiding this comment.
Any reason we shouldn't remove the listeners block from the virtual nodes in this example?
EDIT: Its still necessary.
| @@ -198,7 +201,7 @@ func (v *VirtualNode) Backends() []appmeshv1beta1.Backend { | |||
| return backends | |||
| } | |||
|
|
|||
| // Backends converts into a Set of Backends | |||
| // BackendsSet returns a set of Backends defined for virtual-node | |||
| @@ -208,7 +211,7 @@ func (v *VirtualNode) BackendsSet() set.Set { | |||
| return s | |||
| } | |||
|
|
|||
| // CreateVirtualNode calls describe virtual node. | |||
| // GetVirtualNode calls describe virtual node. | |||
| for _, listener := range vrouter.Listeners { | ||
| listeners = append(listeners, &appmesh.VirtualRouterListener{ | ||
| PortMapping: &appmesh.PortMapping{ | ||
| Port: &listener.PortMapping.Port, |
There was a problem hiding this comment.
Nit: aws.Int64(listener.PortMapping.Port), for consistency with the rest of the file.
|
Ha, started this review yesterday and didn't check to see it was merged :D |
|
Sorry @nckturner, didn't want to bother you with it this week 😄 |
Issue #, if available:
#37
Description of changes:
VirtualRouter allows listener to be configured hence this update to CRD.
Also includes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.