Config options fix#2321
Conversation
Signed-off-by: Shmuel Kallner <kallner@il.ibm.com>
Signed-off-by: Shmuel Kallner <kallner@il.ibm.com>
Signed-off-by: Shmuel Kallner <kallner@il.ibm.com>
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| } | ||
| } | ||
|
|
||
| if opts.ConfigText == "" && opts.ConfigFile == "" { |
There was a problem hiding this comment.
Q: is it (or should it be) possible to run EPP without a configuration file? There are runner options that set config directly in code (With...()). Sould the validation also check the configuration pointers on the runner?
There was a problem hiding this comment.
Ideally we should have a default configuration. I think in general our config defaulting, especially the way individual plugins config defaulting is done needs re-work.
There was a problem hiding this comment.
practically atm we provide default plugins for certain extension points.
we don't provide default configuration if non was specified.
as an immediate fix for the segfault this looks good from my side.
longer term I agree default configuration would be a nicer solution.
There was a problem hiding this comment.
The current With...() functions of the runner do not set enough configuration related stuff.
When the datalayer goes production, there will be more work done.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nirrozenbaum, shmuelk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
/hold cancel |
|
I have created issue #2324 to discuss and track the adding of a default configuration for the EPP. |
|
/cherrypick release-1.3 |
|
@kfswain: #2321 failed to apply on top of branch "release-1.3": DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
* Added check that one of confi-text or config-file is specified Signed-off-by: Shmuel Kallner <kallner@il.ibm.com> * Added dummy config-file argument to get tests to pass Signed-off-by: Shmuel Kallner <kallner@il.ibm.com> * Add a very basic logger to log startup messages in case of errors Signed-off-by: Shmuel Kallner <kallner@il.ibm.com> --------- Signed-off-by: Shmuel Kallner <kallner@il.ibm.com>
* Added check that one of confi-text or config-file is specified Signed-off-by: Shmuel Kallner <kallner@il.ibm.com> * Added dummy config-file argument to get tests to pass Signed-off-by: Shmuel Kallner <kallner@il.ibm.com> * Add a very basic logger to log startup messages in case of errors Signed-off-by: Shmuel Kallner <kallner@il.ibm.com> --------- Signed-off-by: Shmuel Kallner <kallner@il.ibm.com>
What type of PR is this?
/kind bug
What this PR does / why we need it:
A textual configuration is required by the EPP to run.
If neither the config-text nor the config-file options are specified, the EPP crashes with a nil pointer error.
This PR adds to the options validation code a check that one of the config-text or config-file command line arguments has been specified.
In addition this PR adds a very simple log setup for the initial log messages issued before the command line arguments parse and validate correctly. Without this, no error messages show up in the EPP's stdout. See https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/log#hdr-The_Log_Handle for more details.
Which issue(s) this PR fixes:
Fixes #2297
Does this PR introduce a user-facing change?: