giantswarm / ingress-operator

The ingress-operator connects control plane ingress controllers with tenant cluster ingress controllers on a Giant Swarm Kubernetes installation.
2 stars 1 forks source link

add missing methods from the framework.Resource interface #20

Closed fgimenez closed 7 years ago

fgimenez commented 7 years ago

Also vendor and tests are updated. The github.com/giantswarm/ingresstpr dependency is still pointing to the structure branch, this should be updated to master once https://github.com/giantswarm/ingresstpr/pull/6 lands. There are separate commits for the dependencies and code changes.

fgimenez commented 7 years ago

Also worth mentioning, the PR is targetted to the structure branch in https://github.com/giantswarm/ingress-operator/pull/19

kopiczko commented 7 years ago

Oh, and make PR to master instead to structure. That way you can split the changes.

fgimenez commented 7 years ago

I don't think K8s client is actually used in those tests. Did you try running that w/o creating testConfig?

yep, i got this:

{"caller":"github.com/giantswarm/ingress-operator/vendor/github.com/giantswarm/operatorkit/client/k8s/k8s.go:99","debug":"creating in-cluster config","time":"17-09-06 10:06:43.336"}
--- FAIL: Test_Service_GetDesiredState (0.00s)
panic: unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined [recovered]
    panic: unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined

goroutine 13 [running]:
testing.tRunner.func1(0xc4200be1e0)
    /home/fgimenez/src/go/src/testing/testing.go:711 +0x2d2
panic(0x126e960, 0xc420440e60)
    /home/fgimenez/src/go/src/runtime/panic.go:491 +0x283
github.com/giantswarm/ingress-operator/service/resource/configmap.DefaultConfig(0x11dfa20, 0xc4202b4510, 0x1349572, 0x5)
    /home/fgimenez/workspace/gocode/src/github.com/giantswarm/ingress-operator/service/resource/configmap/service.go:48 +0x15b
github.com/giantswarm/ingress-operator/service/resource/configmap.Test_Service_GetDesiredState(0xc4200be1e0)
    /home/fgimenez/workspace/gocode/src/github.com/giantswarm/ingress-operator/service/resource/configmap/service_test.go:94 +0x5e4
testing.tRunner(0xc4200be1e0, 0x13afe08)
    /home/fgimenez/src/go/src/testing/testing.go:746 +0xd0
created by testing.(*T).Run
    /home/fgimenez/src/go/src/testing/testing.go:789 +0x2de
FAIL    github.com/giantswarm/ingress-operator/service/resource/configmap   0.074s

The DefaultConfig call here https://github.com/giantswarm/ingress-operator/blob/master/service/resource/service/service_test.go#L121 causes this.

Also microloggertest.New() come in handy here: https://github.com/giantswarm/micrologger/blob/master/microloggertest/logger.go#L10

Nice! thx a lot, updating

fgimenez commented 7 years ago

Besides the comments left I'd like this change to be split into two:

creating the missing framework.Resource methods updating to new ingresstpr

@kopiczko mm the update to new ingresstpr changes are already in #19, right? i can propose a branch with the required vendor updates to it, however tests won't pass (it won't even compile i think), they need the missing framework.Resource methods, after fixing vendor in #19 i was getting:

$ go test $(glide novendor)
?       github.com/giantswarm/ingress-operator/flag [no test files]
?       github.com/giantswarm/ingress-operator/flag/service [no test files]
?       github.com/giantswarm/ingress-operator/flag/service/kubernetes  [no test files]
?       github.com/giantswarm/ingress-operator/flag/service/kubernetes/tls  [no test files]
# github.com/giantswarm/ingress-operator/service/resource/configmap
service/resource/configmap/service.go:289:2: cannot use s (type *Service) as type framework.Resource in return argument:
    *Service does not implement framework.Resource (missing GetUpdateState method)
# github.com/giantswarm/ingress-operator/service/resource/service
service/resource/service/service.go:287:2: cannot use s (type *Service) as type framework.Resource in return argument:
    *Service does not implement framework.Resource (missing GetUpdateState method)
# github.com/giantswarm/ingress-operator/service/resource/configmap
service/resource/configmap/service.go:289:2: cannot use s (type *Service) as type framework.Resource in return argument:
    *Service does not implement framework.Resource (missing GetUpdateState method)
# github.com/giantswarm/ingress-operator/service/resource/service
service/resource/service/service.go:287:2: cannot use s (type *Service) as type framework.Resource in return argument:
    *Service does not implement framework.Resource (missing GetUpdateState method)

So not sure if those changes can be proposed separately to master, WDYT?

kopiczko commented 7 years ago

@fgimenez don't worry about #19. Let's fix the missing framework.Resource methods first.

fgimenez commented 7 years ago

@kopiczko ok, proposed in #21