Skip to content

Conversation

nirrozenbaum
Copy link
Contributor

@nirrozenbaum nirrozenbaum commented Jul 6, 2025

This PR does minor refactoring to config loading from file/text.
the following changes are included:

  • when working with sets, the best practice in k8s is to use "k8s.io/apimachinery/pkg/util/sets", which is backed by map[T]struct{}, but is more readable. updated config loader to use k8s sets.
  • config loader doesn't get both text (as bytes) and file anymore. this should be handled in the caller and the config loader gets bytes after the caller converted the file or text into bytes.
    **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.
  • config loader gets as input the configBytes and the handle to add the plugins. I noticed that LoadConfig and LoadPluginReferences are always called together one after the other, the separation into two functions wasn't making sense.
  • updated tests accordingly.

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.

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 6, 2025
@k8s-ci-robot k8s-ci-robot requested a review from kfswain July 6, 2025 07:40
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 6, 2025
@k8s-ci-robot k8s-ci-robot requested a review from robscott July 6, 2025 07:40
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 6, 2025
Copy link

netlify bot commented Jul 6, 2025

Deploy Preview for gateway-api-inference-extension ready!

Name Link
🔨 Latest commit 190946b
🔍 Latest deploy log https://app.netlify.com/projects/gateway-api-inference-extension/deploys/686a4f7142132b000819be24
😎 Deploy Preview https://deploy-preview-1114--gateway-api-inference-extension.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Signed-off-by: Nir Rozenbaum <nirro@il.ibm.com>
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 6, 2025
@nirrozenbaum nirrozenbaum changed the title use k8s sets instead of map[string]struct{} minor refactor to config loading from file Jul 6, 2025
@nirrozenbaum nirrozenbaum changed the title minor refactor to config loading from file minor refactor to config loading from file/text Jul 6, 2025
@kfswain
Copy link
Collaborator

kfswain commented Jul 8, 2025

/lgtm

I noticed the request control plugins are handled separately, is that temporary?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2025
@k8s-ci-robot k8s-ci-robot merged commit fe34b90 into kubernetes-sigs:main Jul 8, 2025
9 checks passed
@nirrozenbaum
Copy link
Contributor Author

/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.

@nirrozenbaum nirrozenbaum deleted the k8s-sets branch July 8, 2025 20:31
EyalPazz pushed a commit to EyalPazz/gateway-api-inference-extension that referenced this pull request Jul 9, 2025
Signed-off-by: Nir Rozenbaum <nirro@il.ibm.com>
BenjaminBraunDev pushed a commit to BenjaminBraunDev/gateway-api-inference-extension that referenced this pull request Aug 12, 2025
Signed-off-by: Nir Rozenbaum <nirro@il.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants