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

WIP: add objectfilters #228

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

duncan485
Copy link

I ran into the issue that we cannot set resourceNames in (cluster)Role.rules.resourceNames because the cache/watch includes all the object of a kind in the selected namespace. This PR will limit the cache to only the secrets and webhooks that are needed for the cert-controller.

I tested this locally and verified it works, any feedback would be much appreciated.;

Signed-off-by: duncan485 <bakkerduncan@gmail.com>
@duncan485
Copy link
Author

Thanks @JaydipGabani , I'd say this also closes #24

@duncan485
Copy link
Author

@JaydipGabani Can this one be merged?

@JaydipGabani
Copy link
Contributor

@maxsmythe @ritazh @sozercan PTAL

@JaydipGabani
Copy link
Contributor

@JaydipGabani Can this one be merged?

waiting for one more review at least.

@sozercan sozercan requested a review from maxsmythe August 8, 2024 21:45
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.83%. Comparing base (190188d) to head (5f2f330).
Report is 26 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
+ Coverage   57.16%   62.83%   +5.67%     
==========================================
  Files           1        1              
  Lines         572      479      -93     
==========================================
- Hits          327      301      -26     
+ Misses        181      114      -67     
  Partials       64       64              
Flag Coverage Δ
unittests 62.83% <100.00%> (+5.67%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Mapper: mgr.GetRESTMapper(),
DefaultNamespaces: namespaces,
})
ObjectFilers := make(map[client.Object]cache.ByObject)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ObjectFilers := make(map[client.Object]cache.ByObject)
objectFilers := make(map[client.Object]cache.ByObject)

}
}

ObjectFilers[&corev1.Secret{}] = cache.ByObject{
Copy link
Member

Choose a reason for hiding this comment

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

@duncan485 can we add tests for this?

@duncan485 duncan485 changed the title add objectfilters WIP: add objectfilters Sep 13, 2024
@duncan485
Copy link
Author

While writing tests, I found that this doesn't work with multiple webhooks, it will only add the last added webhook in the filter.

I'm putting this back in WIP, and continue this when I get to it.

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.

5 participants