-
Notifications
You must be signed in to change notification settings - Fork 297
Refactor conf with flatten #7761
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
base: unstable
Are you sure you want to change the base?
Conversation
| name: "rest-max-body-size" .}: Natural | ||
|
|
||
| maxHeadersSize* {. | ||
| defaultValue: 128 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note BeaconNodeconf had this set to 64.
| defaultValue: 8008 | ||
| name: "metrics-port" .}: Port | ||
|
|
||
| MetricsBetaConf* = object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this exists because of the port difference and the description adding "BETA". But maybe can just use MetricsConf
1954583 to
a2846fd
Compare
|
If you going to refactor configs it would be nice if you split all the configs between parts, because when VC builds it imports whole BN codebase just because of the |
| if t.nanoseconds < 0: 0'i64 else: t.nanoseconds)) | ||
|
|
||
| type | ||
| RequestConf* = object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Little nit, but if we pack this perhaps the naming should be a bit more clear that this is about REST API configuration settings. RequestConf seems a bit too general.
example: RestRequestConf or RestApiConf ?
Changes:
MetricsConfrestRequestTimeout,restMaxRequestBodySize,restMaxRequestHeadersSizeintoRequestConf. But this could be passed individually instead.