-
Notifications
You must be signed in to change notification settings - Fork 180
minor refactor to config loading from file/text #1114
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
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nirrozenbaum The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: Nir Rozenbaum <nirro@il.ibm.com>
/lgtm I noticed the request control plugins are handled separately, is that temporary? |
@kfswain yes, noticed that too. this was the behavior before this PR and I was wondering about it as well. I think there is room for improvements (the point you raised and few more points), which can be done in follow up PRs. |
Signed-off-by: Nir Rozenbaum <nirro@il.ibm.com>
Signed-off-by: Nir Rozenbaum <nirro@il.ibm.com>
This PR does minor refactoring to config loading from file/text.
the following changes are included:
"k8s.io/apimachinery/pkg/util/sets"
, which is backed by map[T]struct{}, but is more readable. updated config loader to use k8s sets.**generally speaking, I'm not sure we need to maintain the two options and using a filepath seems sufficient and more aligned with k8s (e.g., mounting configmap to file system). but this is open for discussion and not part of this PR.
LoadConfig
andLoadPluginReferences
are always called together one after the other, the separation into two functions wasn't making sense.there are more improvements that can be done, e.g., load scheduling profiles with the general config loading. there is no good reason to separate. but this is out of scope of this PR.