Skip to content

[gateway] turn API into a trait#6145

Merged
sunshowers merged 2 commits into
mainfrom
sunshowers/spr/gateway-turn-api-into-a-trait
Jul 26, 2024
Merged

[gateway] turn API into a trait#6145
sunshowers merged 2 commits into
mainfrom
sunshowers/spr/gateway-turn-api-into-a-trait

Conversation

@sunshowers

Copy link
Copy Markdown
Contributor

Somewhat large, but with no functional changes -- this is pure code movement.

In a few spots I had to move away from From impls, so that gateway-types does
not depend on gateway-sp-comms. I think those changes are for the best.

As a bonus, sp-sim no longer has to depend on omicron-gateway, cutting down the
number of build units for it from 600 to 468. That's a really nice improvement.

Created using spr 1.3.6-beta.1
Comment thread openapi/gateway.json
Comment on lines +878 to +879
"summary": "Detach the websocket connection attached to the given SP component's",
"description": "serial console, if such a connection exists.",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, Dropshot should probably consider the first paragraph (or maybe the first sentence?) to be the summary -- line breaks affecting this is a bit weird.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First paragraph would match the old way of defining endpoints. We rely on this heavily in Nexus.

@david-crespo david-crespo Jul 23, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like the logic might have gotten stepped on here:

https://github.com/oxidecomputer/dropshot/pull/1008/files#diff-e164752baf582e03498739811eea1ef8039fabafcbb1ef954a5c0cb8d479cef2L17-L78

Edit: or not. The code moved but the tests are the same so it must still work. I guess this must be a difference in implementation in the new macro.

@sunshowers sunshowers Jul 23, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

First paragraph would match the old way of defining endpoints. We rely on this heavily in Nexus.

Hmm, well even before this change it was splitting on the first line break, right? Strange, will look tomorrow.

I tried to keep this bit of code identical between the two macros, but might have missed something.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh yeah, I see you're right: the existing code also worked that way. We just have been diligent about using it correctly in the external API, largely because if we don't, we see the consequence on the docs site — the summaries are used as the sidebar link titles.

Not sure the basic behavior needs to change. Might be enough to just make it easier to notice you've messed up, like with a warning of some kind.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, I see you're right: the existing code also worked that way. We just have been diligent about using it correctly in the external API, largely because if we don't, we see the consequence on the docs site — the summaries are used as the sidebar link titles.

Ahh -- the first line acts as the sidebar title so it's deliberately kept very short for that reason.

I think it's worth exploring better splitting algorithms for sure.

@jgallagher jgallagher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With the caveat that I didn't scrutinize the changes where it looks like things were just being moved around, this looks good to me. Thank you for the better organization under gateway-types than we had previously in gateway!

@sunshowers sunshowers merged commit f5ab051 into main Jul 26, 2024
@sunshowers sunshowers deleted the sunshowers/spr/gateway-turn-api-into-a-trait branch July 26, 2024 22:50
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