Exposing ServerConfig options #959
Conversation
|
Thanks for this! I'm glad to finally resolve that I would eliminate the first breaking change by having Could you also update CHANGELOG.adoc, adding a new "Breaking Changes" section under "Unreleased changes" that points to this issue and explains the extra default values? |
|
Thanks for the review! I updated it back to taking a config by reference and just cloned before passing to the inner server. (Now a much simple PR) Breaking change note has been added. Let me know if the tone of it makes sense. |
| @@ -49,6 +50,10 @@ pub struct ConfigDropshot { | |||
| pub bind_address: SocketAddr, | |||
| /// maximum allowed size of a request body, defaults to 1024 | |||
| pub request_body_max_bytes: usize, | |||
| /// maximum size of any page of results | |||
There was a problem hiding this comment.
do we want to add #[non_exhaustive] to the struct? Would that force consumers to use ..Default::default() when constructing this? i.e. such that adding more fields would not be a breaking change?
There was a problem hiding this comment.
I wondered the same thing. Consumers can already do that if they want. It's kind of nice that the way it is now you can have your code break if we add some new config option so that you get a chance to look yourself at whether our default works for you. I think I've used this in the past.
| @@ -15,6 +15,10 @@ | |||
|
|
|||
| https://github.com/oxidecomputer/dropshot/compare/v0.10.0\...HEAD[Full list of commits] | |||
|
|
|||
| === Breaking Changes | |||
|
|
|||
| * https://github.com/oxidecomputer/dropshot/pull/959[#959] expands the settings available via `ConfigDropshot`. This adds two new fields `page_max_nitems` and `page_default_nitems` that will need to be added wherever a `ConfigDropshot` struct is manually constructed. | |||
There was a problem hiding this comment.
| * https://github.com/oxidecomputer/dropshot/pull/959[#959] expands the settings available via `ConfigDropshot`. This adds two new fields `page_max_nitems` and `page_default_nitems` that will need to be added wherever a `ConfigDropshot` struct is manually constructed. | |
| * https://github.com/oxidecomputer/dropshot/pull/959[#959] expands the settings available via `ConfigDropshot`. This adds two new fields `page_max_nitems` and `page_default_nitems` that will need to be added wherever a `ConfigDropshot` struct is manually constructed. The defaults are likely appropriate for most use cases. |
| @@ -480,7 +480,7 @@ impl<Context: ServerContext> TestContext<Context> { | |||
| pub fn new( | |||
| api: ApiDescription<Context>, | |||
| private: Context, | |||
| config_dropshot: &ConfigDropshot, | |||
| config_dropshot: ConfigDropshot, | |||
There was a problem hiding this comment.
Do we have to change this? This seems like another breaking change. (I'm not sure how this still compiles?)
I wanted to start a discussion on how we might go about exposing some additional server settings to the end user, namely the pagination default values. This PR is the most straightforward take and does not consider backwards compatibility. The main changes are:
ServerConfigwas usedConfigDropshotis now usedConfigDropshotduring construction, the value is passed.ConfigDropshot.page_max_nitemsandpage_default_nitems.This causes two breaking changes:
Defaultimplementation will have to add the new fields.The are a couple options we could also consider here.
ConfigDropshotalready derives Clone. We could continue accepting a reference and clone the users config. Not as ideal, but one less breaking change.ConfigDropshotstruct, we could consider a builder for it. I'm not sure how helpful this is at the moment given theDefaultimpl.Either way the primary goal here is to come up with a pattern to expose some of these configurations to end users at the server level (specifically ignoring the ability to control these settings per endpoint).