search

wave-k8s / wave

Kubernetes configuration tracking controller
Apache License 2.0
661 stars 81 forks source link

Refactoring to support other pod controller types #38

Closed SteveKMin closed 5 years ago

SteveKMin commented 5 years ago

This is the first step in fixing issue https://github.com/pusher/wave/issues/22.

This refactors to wrap the deployment object in a new interface, PodController. This follows @JoelSpeed 's guidance, with a minor change. The interface defines Get and SetPodTemplate that uses the corev1.PodTemplateSpec resource instead. This is to support iterating through template specs other than just Deployment types.

I was not able to run the tests locally. I ran into this error: 2019/03/07 14:39:31 fork/exec /usr/local/kubebuilder/bin/etcd: no such file or directory

rmb938 commented 5 years ago

@JoelSpeed A few changes have been made as well as a bug fix. Could you take a look at this again? Thanks :)

SteveKMin commented 5 years ago

@JoelSpeed I think I've incorporated the requested changes

SteveKMin commented 5 years ago

@JoelSpeed Have you been able to review it?

rmb938 commented 5 years ago

@JoelSpeed Can you take a look at this? We've made the changes you requested.

JoelSpeed commented 5 years ago

Hey al, could you please rebase this PR on top of our master and fix the conflicts and I'll get the CI to run once you've done that, thanks 😄

JoelSpeed commented 5 years ago

/ok-to-test

JoelSpeed commented 5 years ago

@SteveKMin Failing to compile at the moment because of that breaking change, do you think you could fix that up?

You might need to run dep ensure --vendor-only to bring your local dependencies up to date to be able to build locally

SteveKMin commented 5 years ago 
# github.com/pusher/wave/pkg/core
pkg/core/children.go:206:28: cannot use inNamespace (type client.ListOptionFunc) as type runtime.Object in argument to h.Client.List:
    client.ListOptionFunc does not implement runtime.Object (missing DeepCopyObject method)
pkg/core/children.go:206:28: cannot use configMaps (type *"github.com/pusher/wave/vendor/k8s.io/api/core/v1".ConfigMapList) as type client.ListOptionFunc in argument to h.Client.List
pkg/core/children.go:213:27: cannot use inNamespace (type client.ListOptionFunc) as type runtime.Object in argument to h.Client.List:
    client.ListOptionFunc does not implement runtime.Object (missing DeepCopyObject method)
pkg/core/children.go:213:27: cannot use configMaps (type *"github.com/pusher/wave/vendor/k8s.io/api/core/v1".ConfigMapList) as type client.ListOptionFunc in argument to h.Client.List

I think my inNamespace change broke it? I can revert it, but on my machine inNamespace is getting passed to list as a *ListOptions not runtime.Object

JoelSpeed commented 5 years ago

You just need to swap the order of your arguments in the List calls and it should build.

Also try the dep thing -> https://github.com/pusher/wave/pull/38#issuecomment-479521529

SteveKMin commented 5 years ago

Apologizes, I think we commented at the same time. The dep ensure --vendor-only worked.

JoelSpeed commented 5 years ago

Apologizes, I think we commented at the same time. The dep ensure --vendor-only worked.

Haha, yeah I realised, sorry if I came across a bit harsh there 😅

Also, woo, tests are passing, let me do one last check over this and then hopefully we can get it merged! Thanks for your patience and apologies for taking a while, we wanted to hold all PRs until we got our new CI set up and this repo was the second to be migrated

JoelSpeed commented 5 years ago

/build docker

Just gonna build an image and deploy to a real cluster for some quick testing

pusher-ci commented 5 years ago

@SteveKMin: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-wave-build-docker 01f02cc31c6817a44c29f8d937f5aa92cec3d030 link /build docker

Full PR test history

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
JoelSpeed commented 5 years ago

Well that's not good... but also not your fault! I hate that damn azure dependency 😞

SteveKMin commented 5 years ago

:( Also I have a question about the new makefile. What is suppose to be contained in the .env file?

JoelSpeed commented 5 years ago

I'm working on fixing the azure dependency, I think this got broken in #40

The .env points to local tools allowing you to select which version of binaries are used in tests. There is a configure script that you can use to set it up. If you have any missing dependencies then touch .env && make prepare-env-1.13 should be able to fix that for you 😄

The idea behind this is that you can also test against 1.11 and 1.12 but calling the relevant prepare-env-% target

SteveKMin commented 5 years ago

Sounds good, thanks!