Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Use custom informer-gen that supports filters #79

Merged
merged 4 commits into from
Nov 1, 2017

Conversation

munnerz
Copy link
Contributor

@munnerz munnerz commented Oct 31, 2017

This PR switches us to use munnerz/code-generator temporarily, which includes the changes in kubernetes/kubernetes#54660. I've then updated our codegen scripts to also generate informers for internal kubernetes types (although not a client or lister, as we can re-use client-go for this).

I'm hoping my PR will be merged soon, at which point we can remove the alternate repo source, but for now this lets us clear up the mess that is our SharedInformerFactory.

NONE

Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@munnerz The plan makes sense.

A quick thougt. Could we instead maintain a Jetstack fork of the code-generator and point dep at that.
That's how I've seen this done in Python world.
Or is that going to be a massive pain with GOPATH?

@munnerz
Copy link
Contributor Author

munnerz commented Oct 31, 2017

Yeah that is what I hoped to do originally. dep isn't able to handle that, and actually their justifications are fair (if two packages both depend on the same package, but one of them specifies a different 'source', what should happen, as we can't check out code from two places so the dependency cannot be met).

It'd be nice if there were some way to do it, but doesn't seem like it.

I've actually just run into a new problem, which is that dep wipes out the vendor/ dir upon dep ensure, so I've had to actually update the Makefile to include a ln -s which is not the best. I'm really hoping my PR gets accepted upstream so we don't have to do this forever! There's been some good/positive discussion on it, so I fingers crossed 😄

@wallrj
Copy link
Member

wallrj commented Nov 1, 2017

Hey @munnerz

I just read: https://github.com/golang/dep/blob/master/docs/Gopkg.toml.md#constraint

[[constraint]]
...
  # Optional: an alternate location (URL or import path) for the project's source.
  source = "https://github.com/myfork/package.git"

Found via the response to this comment: golang/dep#248 (comment)

@munnerz
Copy link
Contributor Author

munnerz commented Nov 1, 2017

Thanks @wallrj! Missed that!!

@munnerz
Copy link
Contributor Author

munnerz commented Nov 1, 2017

@wallrj this is now passing! 🎉

I've also opened a follow up PR #82 that switches the ElasticsearchController to use these new factories.

Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @munnerz

Looks great! I looked at #82 and the new factory code looks much neater.

I think we can delete the third_party stuff now.

Otherwise

/lgtm

@@ -45,6 +45,8 @@ required = [
name = "k8s.io/apiserver"
branch = "release-1.8"

# Use munnerz/code-generator fork that includes the filter changes
[[constraint]]
name = "k8s.io/code-generator"
branch = "release-1.8"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ I was going to ask if this is the correct branch in your forked repo, but it looks like you've applied the changes there too: https://github.com/munnerz/code-generator/tree/release-1.8

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - otherwise there were more changes to generated files that were unrelated. Thought it best to make this 1:1

--input-dirs "k8s.io/api/apps/v1beta1" \
--versioned-clientset-package "k8s.io/client-go/kubernetes" \
--listers-package "k8s.io/client-go/listers" \
--output-package "github.com/jetstack/navigator/third_party/k8s.io/client-go/informers" \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Are we still using the third_party sub-directory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, but not with any vendor hacks this time (symlinks).

This is just a place to store the generated filter-able informers from the main k8s codebase.

@munnerz
Copy link
Contributor Author

munnerz commented Nov 1, 2017

/approve

@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz, wallrj

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@jetstack-bot
Copy link
Collaborator

Automatic merge from submit-queue.

@jetstack-bot jetstack-bot merged commit ee896bb into jetstack:master Nov 1, 2017
jetstack-bot added a commit that referenced this pull request Nov 1, 2017
Automatic merge from submit-queue.

Use filtered SharedInformerFactories in controller context

**What this PR does / why we need it**:

Switches to using the new filtered SharedInformerFactories as part of kubernetes/kubernetes#54660

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #

fixes #69 

**Special notes for your reviewer**:

Don't merge until #79 has been merged

**Release note**:

```release-note
NONE
```

/assign @wallrj
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants