Skip to content
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

remote_storage config: move handling of empty inline table {} to callers #8193

Merged
merged 6 commits into from
Jul 2, 2024

Conversation

problame
Copy link
Contributor

Before this PR, RemoteStorageConfig::from_toml would support deserializing an
empty {} TOML inline table to a None, otherwise try Some().

We can instead let

  • in proxy: let clap derive handle the Option
  • in PS & SK: assume that if the field is specified, it must be a valid
    RemtoeStorageConfig

(This PR started with a much simpler goal of factoring out the
deserialize_item function because I need that in another PR).

…llers

Before this PR, `RemoteStorageConfig::from_toml` would support deserializing an
empty `{}` TOML inline table to a `None`, otherwise try `Some()`.

We can instead let
* in proxy: let clap derive handle the Option
* in PS & SK: assume that if the field is specified, it must be a valid
  RemtoeStorageConfig

(This PR started with a much simpler goal of factoring out the
`deserialize_item` function because I need that in another PR).
Copy link

github-actions bot commented Jun 28, 2024

3000 tests run: 2885 passed, 0 failed, 115 skipped (full report)


Code coverage* (full report)

  • functions: 32.7% (6914 of 21147 functions)
  • lines: 50.0% (54214 of 108324 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
52011d7 at 2024-07-02T09:59:05.806Z :recycle:

@problame problame marked this pull request as ready for review June 28, 2024 12:40
@problame problame requested review from a team as code owners June 28, 2024 12:40
@problame
Copy link
Contributor Author

problame commented Jul 1, 2024

Started a thread with cplane team on the e2e failures here: https://neondb.slack.com/archives/C03438W3FLZ/p1719844236785699

@problame problame merged commit 7dcdbaa into main Jul 2, 2024
64 checks passed
@problame problame deleted the problame/refactor-remote-storage-parsing branch July 2, 2024 10:53
VladLazar pushed a commit that referenced this pull request Jul 8, 2024
…llers (#8193)

Before this PR, `RemoteStorageConfig::from_toml` would support
deserializing an
empty `{}` TOML inline table to a `None`, otherwise try `Some()`.

We can instead let
* in proxy: let clap derive handle the Option
* in PS & SK: assume that if the field is specified, it must be a valid
  RemtoeStorageConfig

(This PR started with a much simpler goal of factoring out the
`deserialize_item` function because I need that in another PR).
VladLazar pushed a commit that referenced this pull request Jul 8, 2024
…llers (#8193)

Before this PR, `RemoteStorageConfig::from_toml` would support
deserializing an
empty `{}` TOML inline table to a `None`, otherwise try `Some()`.

We can instead let
* in proxy: let clap derive handle the Option
* in PS & SK: assume that if the field is specified, it must be a valid
  RemtoeStorageConfig

(This PR started with a much simpler goal of factoring out the
`deserialize_item` function because I need that in another PR).
VladLazar pushed a commit that referenced this pull request Jul 8, 2024
…llers (#8193)

Before this PR, `RemoteStorageConfig::from_toml` would support
deserializing an
empty `{}` TOML inline table to a `None`, otherwise try `Some()`.

We can instead let
* in proxy: let clap derive handle the Option
* in PS & SK: assume that if the field is specified, it must be a valid
  RemtoeStorageConfig

(This PR started with a much simpler goal of factoring out the
`deserialize_item` function because I need that in another PR).
VladLazar pushed a commit that referenced this pull request Jul 8, 2024
…llers (#8193)

Before this PR, `RemoteStorageConfig::from_toml` would support
deserializing an
empty `{}` TOML inline table to a `None`, otherwise try `Some()`.

We can instead let
* in proxy: let clap derive handle the Option
* in PS & SK: assume that if the field is specified, it must be a valid
  RemtoeStorageConfig

(This PR started with a much simpler goal of factoring out the
`deserialize_item` function because I need that in another PR).
VladLazar pushed a commit that referenced this pull request Jul 8, 2024
…llers (#8193)

Before this PR, `RemoteStorageConfig::from_toml` would support
deserializing an
empty `{}` TOML inline table to a `None`, otherwise try `Some()`.

We can instead let
* in proxy: let clap derive handle the Option
* in PS & SK: assume that if the field is specified, it must be a valid
  RemtoeStorageConfig

(This PR started with a much simpler goal of factoring out the
`deserialize_item` function because I need that in another PR).
VladLazar pushed a commit that referenced this pull request Jul 8, 2024
…llers (#8193)

Before this PR, `RemoteStorageConfig::from_toml` would support
deserializing an
empty `{}` TOML inline table to a `None`, otherwise try `Some()`.

We can instead let
* in proxy: let clap derive handle the Option
* in PS & SK: assume that if the field is specified, it must be a valid
  RemtoeStorageConfig

(This PR started with a much simpler goal of factoring out the
`deserialize_item` function because I need that in another PR).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants