Skip to content

Conversation

sunshowers
Copy link
Contributor

@sunshowers sunshowers commented Sep 26, 2025

Created using spr 1.3.6-beta.1
@sunshowers sunshowers changed the title migrate to dropshot-api-manager migrate to Dropshot API manager Sep 26, 2025
@sunshowers sunshowers marked this pull request as draft September 26, 2025 23:29
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
@sunshowers sunshowers marked this pull request as ready for review September 30, 2025 20:47
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
#[clap(name = "dpd")]
pub(crate) enum Args {
/// Run the Dendrite API server.
Run(Box<Opt>),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we only have the run command, we can just drop the subcommand structure entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, we've retained the subcommand structure in other servers even if it's the only command.

If you still think we should get rid of the run command, can this be a followup? Would have to update all the places that run the binary as well, which adds risk for something that's pretty time-constrained.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If our standard practice is to keep the vestigial run subcommand, I'm fine with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks -- see https://github.com/oxidecomputer/omicron/blob/5f7ab65c8c8b215c6cf71b0d6e018574aa2e7f92/sled-agent/src/bin/sled-agent.rs#L25-L31 for an example. I guess one advantage is that if we do need to add a second command in the future, the places that run the binary don't have to be updated again.

@sunshowers sunshowers merged commit d6f5da9 into main Oct 7, 2025
6 checks passed
@sunshowers sunshowers deleted the sunshowers/spr/migrate-to-dropshot-api-manager branch October 7, 2025 20:29
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.

2 participants