Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

namespace apprepo-controller kube informer #787

Merged
merged 3 commits into from
Nov 7, 2018

Conversation

prydonius
Copy link
Contributor

@prydonius prydonius commented Nov 5, 2018

The NewFilteredSharedInformerFactory was not available in previous versions
of client-go we were using. Now that we've upgraded, we can correctly
filter down to the namespace this controller is interested in
(the one Kubeapps is installed in).

chart: remove clusterrole for apprepository-controller

Now that we are able to watch on the specific namespace Kubeapps was
installed in, we no longer need the clusterrole to watch cluster-wide
CronJobs.

fixes #504

Copy link
Contributor

@migmartri migmartri left a comment

Choose a reason for hiding this comment

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

Awesome.

Any test changes required or worth adding?

@prydonius
Copy link
Contributor Author

prydonius commented Nov 5, 2018

@migmartri I'm not sure, the tests are currently for controller.go only, and I don't know if we can mock the shared informers? Even if we did test mocked informers, we'd be passing an initialised informer to the controller, so we won't actually be able to test this particular change. I believe this is something only an e2e test would be able to verify. @andresmgot may have some more ideas though.

@andresmgot
Copy link
Contributor

I don't think we need an unit test for this change (since the call is syntactically correct).

What I don't fully understand is why do we need to listen for CronJobs: Apparently is to be safe and recreate a CronJob that someone deletes manually, am I right? If we really care about that we could add an e2e test to check that the CronJob gets recreated but I don't think it's necessary.

@migmartri
Copy link
Contributor

I don't think we need an unit test for this change (since the call is syntactically correct).

What I don't fully understand is why do we need to listen for CronJobs: Apparently is to be safe and recreate a CronJob that someone deletes manually, am I right? If we really care about that we could add an e2e test to check that the CronJob gets recreated but I don't think it's necessary.

@prydonius, would you mind adding a comment to make it more clear? Thanks!

Adnan Abdulhussein added 3 commits November 6, 2018 14:51
The NewFilteredSharedInformerFactory was not available in previous
versions of client-go we were using. Now that we've upgraded, we can
correctly filter down to the namespace this controller is interested in
(the one Kubeapps is installed in).
Now that we are able to watch on the specific namespace Kubeapps was
installed in, we no longer need the clusterrole to watch cluster-wide
CronJobs.
@prydonius prydonius force-pushed the 504-namespaced-watch branch from 4de55e9 to 16476e6 Compare November 6, 2018 23:25
@prydonius
Copy link
Contributor Author

@migmartri updated the comment to make it more clear

@prydonius prydonius merged commit 9c6ac74 into vmware-tanzu:master Nov 7, 2018
@prydonius prydonius deleted the 504-namespaced-watch branch November 7, 2018 01:07
prydonius pushed a commit to prydonius/kubeapps that referenced this pull request Nov 13, 2018
This reverts commit 9c6ac74, since it
contains a change in the chart that was not yet ready to release and is
currently causing the apprepository-controller fail due to insufficient
permissions.
prydonius added a commit that referenced this pull request Nov 13, 2018
* Revert "namespace apprepo-controller kube informer (#787)"

This reverts commit 9c6ac74, since it
contains a change in the chart that was not yet ready to release and is
currently causing the apprepository-controller fail due to insufficient
permissions.

* bump chart version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update client-go
3 participants