minor changes in few places #702
Conversation
Signed-off-by: Nir Rozenbaum <[email protected]>
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/S
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Nir Rozenbaum <[email protected]>
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, 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 |
k8s-ci-robot
added
the
approved
| // DefaultManagerOptions returns the default options used to create the manager. | ||
| func DefaultManagerOptions(namespace, name string) ctrl.Options { | ||
| // defaultManagerOptions returns the default options used to create the manager. | ||
| func defaultManagerOptions(namespace string, name string) ctrl.Options { |
There was a problem hiding this comment.
why add the extra string type?
There was a problem hiding this comment.
also, typically we place name first and then namespace; one thing to consider is using namespacedName type to avoid mistakenly switching those parameters .
There was a problem hiding this comment.
agree. I will push this shortly.
| @@ -36,9 +35,8 @@ import ( | |||
| // will have the proper controller that will create/manage objects on behalf of the server pool. | |||
| type InferencePoolReconciler struct { | |||
| client.Client | |||
| Record record.EventRecorder | |||
| PoolNamespacedName types.NamespacedName | |||
There was a problem hiding this comment.
Ahhhh since we moved inference model name/namespace handling to the manager we don't use this anymore. KK, just thinking aloud: this should be fine to not document that a name change would make the current EPP depoloyment unusable b/c the k8s name is immutable and they are essentially spinning up another resource, same with the namespace. So we probably don't need to document that behavior.
* minor changes in few places Signed-off-by: Nir Rozenbaum <[email protected]> * removed empty labels field Signed-off-by: Nir Rozenbaum <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]>
* minor changes in few places Signed-off-by: Nir Rozenbaum <[email protected]> * removed empty labels field Signed-off-by: Nir Rozenbaum <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]>
* minor changes in few places Signed-off-by: Nir Rozenbaum <[email protected]> * removed empty labels field Signed-off-by: Nir Rozenbaum <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]>
this PR handles few minor things:
defaultnamespace. having said that, I tried few days ago to follow the getting started guide while deploying to another namespace. overall it went fine, but it took me a while to recall this flag has to be set. in our docs and examples we assume using default namespace, but when users deploy GIE in their envs, this is unlikely. I think poolNamespace flag should be passed explicitly to allow relatively easy way to change the used namespace (I don't expect a newcomer who tries following the getting started guide to get into code and understand they need to add this flag).