Improve options handling #123
Conversation
k8s-ci-robot
added
kind/feature
|
@olivierlemasle: This issue is currently awaiting triage. If custom-metrics-apiserver contributors determine this is a relevant issue, they will accept it by applying the The 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/test-infra repository. |
k8s-ci-robot
added
the
size/L
olivierlemasle
force-pushed
the
server
branch
2 times, most recently
from
275418b to
bd921ba
Compare
pkg/cmd/server/options.go
Outdated
| func (o CustomMetricsAdapterServerOptions) Validate(args []string) error { | ||
| return nil | ||
| // Validate validates CustomMetricsAdapterServerOptions | ||
| func (o CustomMetricsAdapterServerOptions) Validate() error { |
There was a problem hiding this comment.
Just for the sake of being consistent with the kube-apiserver code, I think it would be better to have a return type of []error
There was a problem hiding this comment.
Actually, I used the sample-apiserver as an example (since this file seems to be "forked" from sample-apiserver), and they use this signature:
To which Validate() []error function do you refer?
There was a problem hiding this comment.
I was looking at the recommended options code: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/server/options/recommended.go#L158-L172.
Now that you've mention it is coming from the sample-apiserver I am fine with both but would prefer the recommended options approach since custom-metrics-apiserver is a library (similarly to recommended options) and not a complete apiserver.
There was a problem hiding this comment.
Ok, it makes sense. Actually, this code came from the sample-apiserver, but it was before the introduction of the builder in 35cf4d7, which changed the way it is used.
I'll update the PR with Validate() []error and ApplyTo.
No problem changing the API between versions?
There was a problem hiding this comment.
No problem changing the API between versions?
It needs to be taken into consideration since it will require users to change their implementation a bit, but still, the behavior will not change a lot, they will just need to update the implementation slightly. As long as it is described in the release note, I am fine with it.
Moreover I am thinking that we should try to bring the resource metrics API over before doing a next release so users will need to do a bit more changes this release compare to others so now should be the best time to unify the implementations IMO
There was a problem hiding this comment.
Ok, it makes sense. Actually, this code came from the sample-apiserver, but it was before the introduction of the builder in 35cf4d7, which changed the way it is used.
Make sense to use the newer implementation then.
pkg/cmd/server/options.go
Outdated
| } | ||
|
|
||
| // Config returns configuration for the API server given CustomMetricsAdapterServerOptions | ||
| func (o CustomMetricsAdapterServerOptions) Config() (*apiserver.Config, error) { |
There was a problem hiding this comment.
Since this is meant to be consumed as a library, I would be in favor of updating this method to follow a similar workflow as the options from the kube-apiserver recommended options. The signature would look like:
func (o CustomMetricsAdapterServerOptions) ApplyTo(config *apiserver.Config) error
olivierlemasle
force-pushed
the
server
branch
from
bd921ba to
ca534ae
Compare
|
Removed the changes related to pprof and /remove-kind feature |
k8s-ci-robot
added
kind/cleanup
- Adds validation - Improve code documentation and add tests
olivierlemasle
force-pushed
the
server
branch
from
ca534ae to
3debc97
Compare
|
Pushed a new version to address your comments @dgrisonnet. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgrisonnet, olivierlemasle 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
ApplyFeaturesflags (related to profiling): they were defined but unusedAdds the possibility to disable the APIServer's own metrics (however, I did not add a flag for that)Note: this breaks the API compatibility by:
CustomMetricsAdapterServerOptions.CompleteCustomMetricsAdapterServerOptions.Validatefromfunc([]string) errortofunc() errorHowever, these two functions were empty, so probably unused by downstream implementations. Do you still prefer if I keep the API compatibility ?
/kind feature