Migrate banned users, order events cleanup, and S3 config to config file#4192
Migrate banned users, order events cleanup, and S3 config to config file#4192jmg-duarte merged 1 commit intomainfrom
Conversation
Move banned_users, order_events_cleanup_interval/threshold, and S3 upload settings from CLI arguments to the TOML config file, following the same pattern as prior fee_policy and trusted_tokens migrations. - Add config modules: banned_users, order_events_cleanup, s3 - Remove infra::persistence::cli (S3 CLI args) - Delete corresponding CLI arguments and Display impl entries - Wire config values through run.rs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request successfully migrates banned users, order events cleanup, and S3 configuration from CLI arguments to a TOML configuration file. This improves maintainability and centralizes configuration. However, several new test cases contain a compilation error where a variable named toml shadows the toml crate, preventing calls to toml::from_str. These should be renamed to ensure the test suite remains functional.
| fn default_max_cache_size() -> NonZeroUsize { | ||
| NonZeroUsize::new(10000).unwrap() | ||
| } |
There was a problem hiding this comment.
I wonder if we should have a convention for the ordering of config code now that we have a clean slate here.
To me the order of importance would be:
- structs
- default values
- struct impls
- tests
Feel free to ignore as I don't feel strongly about this.
There was a problem hiding this comment.
In this case I didn't but should've: I usually write the defaults in consts (regular or fns) so I tend place them at the top
So the current migration has been working on defaults -> (struct X -> struct X impl)+ -> tests
|
|
||
| /// Configuration for uploading auction instances to S3. | ||
| /// If absent, S3 uploads are disabled. | ||
| #[serde(default)] |
There was a problem hiding this comment.
No default needed when the value is already optional.
| #[serde(default)] |
There was a problem hiding this comment.
I forgot this, Ill add this to the next PR to avoid more CI time 🙏 my apologies
jmg-duarte
added
the
track:config-migration
3 participants
Description
Move banned_users, order_events_cleanup_interval/threshold, and S3 upload settings from CLI arguments to the TOML config file.
Changes
How to test
Staging