Update Kubernetes deps to use release-1.8 branches #47

munnerz · 2017-10-19T13:33:49Z

No description provided.

munnerz · 2017-10-19T15:16:09Z

Since kubernetes/kubernetes#52186 has merged, we can now really easily build generators and also generate files. We should gradually switch all of our projects over to use these new generate-* scripts.

This PR switches us to use this, fixes us to the release-1.8 branches of the Kubernetes API repositories, and updates the import paths for the newly generated files.

Fix up Makefile

Add note in update-client-gen.sh

wallrj

Thanks @munnerz

I checked out the branch and ran dep ensure -v --dry-run --no-vendor. It reports:

Gopkg.lock was not up to date

I ran make all and got the following error:

vendor/github.com/coreos/etcd/etcdserver/api/v3rpc/interceptor.go:57: cannot use info (type *"github.com/jetstack-experimental/navigator/vendor/google.golang.org/grpc".UnaryServerInfo) as type *"google.golang.org/grpc".UnaryServerInfo in argument to grpc_prometheus.UnaryServerInterceptor
vendor/github.com/coreos/etcd/etcdserver/api/v3rpc/interceptor.go:57: cannot use handler (type "github.com/jetstack-experimental/navigator/vendor/google.golang.org/grpc".UnaryHandler) as type "google.golang.org/grpc".UnaryHandler in argument to grpc_prometheus.UnaryServerInterceptor
vendor/github.com/coreos/etcd/etcdserver/api/v3rpc/interceptor.go:93: cannot use ss (type "github.com/jetstack-experimental/navigator/vendor/google.golang.org/grpc".ServerStream) as type "google.golang.org/grpc".ServerStream in argument to grpc_prometheus.StreamServerInterceptor:
	"github.com/jetstack-experimental/navigator/vendor/google.golang.org/grpc".ServerStream does not implement "google.golang.org/grpc".ServerStream (wrong type for Context method)
		have Context() "github.com/jetstack-experimental/navigator/vendor/golang.org/x/net/context".Context
		want Context() "golang.org/x/net/context".Context
vendor/github.com/coreos/etcd/etcdserver/api/v3rpc/interceptor.go:93: cannot use info (type *"github.com/jetstack-experimental/navigator/vendor/google.golang.org/grpc".StreamServerInfo) as type *"google.golang.org/grpc".StreamServerInfo in argument to grpc_prometheus.StreamServerInterceptor
vendor/github.com/coreos/etcd/etcdserver/api/v3rpc/interceptor.go:93: cannot use handler (type "github.com/jetstack-experimental/navigator/vendor/google.golang.org/grpc".StreamHandler) as type "google.golang.org/grpc".StreamHandler in argument to grpc_prometheus.StreamServerInterceptor
FAIL	github.com/jetstack-experimental/navigator/pkg/registry/navigator/cassandracluster [build failed]
?   	github.com/jetstack-experimental/navigator/pkg/registry/navigator/escluster	[no test files]
?   	github.com/jetstack-experimental/navigator/pkg/registry/navigator/pilot	[no test files]
?   	github.com/jetstack-experimental/navigator/pkg/test/integration/framework	[no test files]
?   	github.com/jetstack-experimental/navigator/pkg/util	[no test files]
?   	github.com/jetstack-experimental/navigator/pkg/util/errors	[no test files]
ok  	github.com/jetstack-experimental/navigator/pkg/util/hash	0.032s
make: *** [Makefile:84: go_test] Error 2

With go version go1.8.4 linux/amd64

I left one or two additional questions and comments above.

Please answer or address at your discretion and then merge.

wallrj · 2017-10-20T15:56:46Z

Gopkg.toml

@@ -30,25 +30,21 @@ required = [
 ]

 [[constraint]]
- name = "github.com/Sirupsen/logrus"
- revision = "v0.6.2-10-g51fe59a"


❓ I guess this is because we've switched to glog in navigator.

Yep - this was just a constraint since we removed use of logrus (the package itself wasn't actually depended upon since before this PR)

wallrj · 2017-10-20T15:57:36Z

Gopkg.toml

 name = "k8s.io/client-go"
- branch = "master"
+ branch = "release-5.0"


❓ Is there any advantage to using a version directive here? Will the release branch be updated with patch releases etc?

Yes, the release branches are updated. We'll need to run dep ensure -update periodically to re-vendor those changes.

wallrj · 2017-10-20T15:59:00Z

Makefile

- $(BINDIR)/client-gen \
- $(BINDIR)/lister-gen \
- $(BINDIR)/informer-gen
- touch $@


❓ So we no longer have to compile these binaries? Where do they get compiled now?

They now get compiled as part of the new script in code-generator. See: kubernetes/kubernetes#52186

wallrj · 2017-10-20T16:01:54Z

cmd/apiserver/start.go

- clientset "github.com/jetstack-experimental/navigator/pkg/client/clientset_generated/internalclientset"
- informers "github.com/jetstack-experimental/navigator/pkg/client/informers_generated/internalversion"
+ clientset "github.com/jetstack-experimental/navigator/pkg/client/clientset/internalversion"
+ informers "github.com/jetstack-experimental/navigator/pkg/client/informers/internalversion"


❓ So how do we decide whether to use the internalversion or the versioned APIs? I guess I can see why the API server its self should use the internal versions, but why shouldn't the controller also use the internal APIs?

Internal types should only be used within the API machinery for the apiserver, or if for some reason you need to convert between two external versions. That said, our controller should be built against one and only one version of the API, so there should basically never be a requirement to convert locally.

The controller shouldn't use the internal types for a number of reasons:

These types can change, breaking compatibility.

If we were to use the internal types, we'd then need to convert back into a versioned type in order to submit it back to the API server. We'd also need to convert to the internal version clientside too.

I'd question what we have to gain from trying to do this

It's also against all best practices for this sort of stuff, and would be a departure from how it is done within the k8s codebase. I can't recall an issue or justification for this decision, but I'm certain there is one with good reason.

There are a number of other potential complexities in doing this, and @dippynark and I did discuss it recently. I've managed to forget most of our thoughts on it though now! I will update here if they come back to me 😄

wallrj · 2017-10-20T16:03:44Z

hack/update-client-gen.sh

-BINDIR=${REPO_ROOT}/bin
-HACKDIR=${HACKDIR:-hack}
+SCRIPT_ROOT=$(dirname ${BASH_SOURCE})/..
+CODEGEN_PKG=${CODEGEN_PKG:-$(cd ${SCRIPT_ROOT}; ls -d -1 ./vendor/k8s.io/code-generator 2>/dev/null || echo ../code-generator)}


❓ I can't quite figure out what this is doing? Does it need an explanatory comment or is this copied over from Kubernetes?

It's copied over from the Kubernetes codebase. Roughly:

Set CODEGEN_PKG to CODEGEN_PKG if it is already set

Otherwise, cd into the root of the repo (SCRIPT_ROOT) and attempt to list ./vendor/k8s.io/code-generator if it exists, piping errors to /dev/null. If this command is successful, ./vendor/k8s.io/code-generator is printed to stdout from ls, thus CODEGEN_PKG becomes ./vendor/k8s.io/code-generator.

Otherwise, return ../code-generator. This final step is to deal with the case where our project is located within $GOPATH/src/k8s.io (or k8s.io/kubernetes/staging/src/k8s.io/). I copied this from the sample-controller project, hence this extra condition 😄

The script itself will error regardless if this path is set incorrectly, so I'm not too bothered about falling back to a directory that likely doesn't exist.

wallrj · 2017-10-20T16:25:46Z

Ah ignore me:

I checked out the branch and ran dep ensure -v --dry-run --no-vendor. It reports:
Gopkg.lock was not up to date
I ran make all and got the following error:

There were some lingering files frrom my cassandra work.

munnerz · 2017-10-22T10:25:10Z

Thanks for the review @wallrj - I've responded to your comments above.

Running dep ensure -v --dry-run --no-vendor locally I get: Gopkg.lock was already in sync with imports and Gopkg.toml. I guess whatever changes you had locally changed that 😄

munnerz added 3 commits October 19, 2017 14:00

Update Kubernetes deps to use release-1.8 branches

f336cac

Use generate-internal-groups.sh script

e0d659f

Regenerate files

1fe4629

munnerz added 3 commits October 20, 2017 16:44

Update import paths

d37bc4e

Fix up Makefile

Fix up pkg/apiserver for changes between master and release-1.8

70d8400

Fix conversion generation without k8s.io/kubernetes present

c6c352a

Add note in update-client-gen.sh

munnerz force-pushed the update-deps branch from b876cf2 to c6c352a Compare October 20, 2017 15:46

wallrj approved these changes Oct 20, 2017

View reviewed changes

munnerz merged commit 35da950 into master Oct 22, 2017

munnerz deleted the update-deps branch October 24, 2017 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Kubernetes deps to use release-1.8 branches #47

Update Kubernetes deps to use release-1.8 branches #47

munnerz commented Oct 19, 2017

munnerz commented Oct 19, 2017

wallrj left a comment

wallrj Oct 20, 2017

munnerz Oct 22, 2017

wallrj Oct 20, 2017

munnerz Oct 22, 2017

wallrj Oct 20, 2017

munnerz Oct 22, 2017

wallrj Oct 20, 2017

munnerz Oct 22, 2017

wallrj Oct 20, 2017

munnerz Oct 22, 2017

wallrj commented Oct 20, 2017

munnerz commented Oct 22, 2017

Update Kubernetes deps to use release-1.8 branches #47

Update Kubernetes deps to use release-1.8 branches #47

Conversation

munnerz commented Oct 19, 2017

munnerz commented Oct 19, 2017

wallrj left a 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

wallrj commented Oct 20, 2017

munnerz commented Oct 22, 2017