-
Notifications
You must be signed in to change notification settings - Fork 20
Builder pattern for EphemeralTestnet allowing for custom configurations #278
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: main
Are you sure you want to change the base?
Conversation
109f1a6 to
ee01a45
Compare
82ab0b4 to
31d664e
Compare
SHAcollision
left a comment
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.
Nice work! I think the only way to know how if this is good to go or more changes are needed is to jump into pubky/pubky-nexus repo, swap pubky-tesnet and pubky deps for the ones on this branch and see how difficult/easy the transition is. I think the transition might not be possible "as is". But it might be worth pinging @ok300 for further review/comments.
| let testnet = EphemeralTestnet::start().await.unwrap(); | ||
| use pubky_testnet::pubky_homeserver::ConfigToml; | ||
| let mut config = ConfigToml::default_test_config(); | ||
| config.admin.enabled = true; // Enable admin server |
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.
The EphemeralTestnet promises an admin server, but builder().build() now always uses ConfigToml::test() (whose default_test_config disables admin and metrics) before starting the homeserver. As a result, EphemeralTestnet::start() returns a network without an admin server unless callers explicitly override the config, which can break existing consumers relying on the prior default.
|
|
||
| me.testnet.create_http_relay().await?; | ||
| me.testnet.create_homeserver().await?; | ||
| impl EphemeralTestnetBuilder { |
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.
The previous start_with_custom_postgres and start_minimal_with_custom_postgres entry points are gone, and the new builder always provisions a homeserver. There is no way to create a minimal network (HTTP relay only) while pointing at a custom Postgres URL, and the start_minimal shortcut cannot take a connection string anymore. This is likely a public API regression for downstream tests/tools that depended on those entry points (e.g. Nexus tests)
|
|
||
| /// Create an additional homeserver with a random keypair | ||
| /// Create an additional homeserver with a random keypair. | ||
| pub async fn create_random_homeserver( |
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.
Homeserver creation helpers disable admin/metrics by default. This changes behaviour for callers that previously might be expecting the extra homeservers to expose admin endpoints out of the box; if that change is intentional it should be documented and the helper name/comment updated, otherwise the defaults should be restored or gated by parameters. I can't recall if the homeserver admin server is ever used or not in any of the Nexus test cases. Maybe it is on the pubky-moderation tests?
Here we implement a builder for
EphemeralTestnetfor the purpose of allowing a homeserver config to be specified.This patterns allows for further customisation possibilities, too.
Right now with this we: