Skip to content

Show transceiver errors in wicket UI #8118

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bnaecker
Copy link
Collaborator

@bnaecker bnaecker commented May 8, 2025

- Make all transceiver data results rather than options
- Report errors in the `wicket` UI for each piece of data we failed to
  read
- Fixes #8110
@bnaecker bnaecker requested review from sunshowers and jgallagher May 8, 2025 18:17
@bnaecker
Copy link
Collaborator Author

bnaecker commented May 8, 2025

I ran a quick test of this by manually inserting a failed transceiver on the wicketd side. It shows up like so in the wicket UI:

Screenshot 2025-05-08 at 11 18 43

There's some duplication of the errors in different lines because it's derived from the same request to the SP for the transceiver state, but I think it's OK. We could reorg the UI more significantly to reduce that if it's a big eyesore.

@bnaecker
Copy link
Collaborator Author

bnaecker commented May 8, 2025

Not sure the best way to handle the OpenAPI linting issues here. I really want a Result, but that ends up with a terrible name in the spec. Hand-rolling enums that are the same, with Ok(T) and Err(String) variants works, but is also kind of annoying for developers.

@jgallagher
Copy link
Contributor

Not sure the best way to handle the OpenAPI linting issues here. I really want a Result, but that ends up with a terrible name in the spec. Hand-rolling enums that are the same, with Ok(T) and Err(String) variants works, but is also kind of annoying for developers.

Hmm. @davepacheco did this recently:

#[serde(serialize_with = "serialize_snake_case_result")]
#[schemars(
schema_with = "SnakeCaseResult::<UpdateCompletedHow, String>::json_schema"
)]
pub result: Result<UpdateCompletedHow, String>,

with this helper:

#[derive(JsonSchema, Serialize)]
#[serde(rename = "Result{T}Or{E}")]
#[serde(rename_all = "snake_case")]
enum SnakeCaseResult<T, E> {
Ok(T),
Err(E),
}
fn serialize_snake_case_result<S, T, E>(
value: &Result<T, E>,
serializer: S,
) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
T: Serialize,
E: Serialize,
{
match value {
Ok(val) => SnakeCaseResult::Ok(val),
Err(err) => SnakeCaseResult::Err(err),
}
.serialize(serializer)
}

I wonder if it makes sense to move that somewhere more public and reuse it?

@bnaecker
Copy link
Collaborator Author

bnaecker commented May 8, 2025

@jgallagher Thanks! I'm not sure how common it will be, but we definitely have an N of 2! @ahl had also mentioned to me a way to convince Progenitor to actually use an explicit Rust type when generating code. In this case, telling it to use the actual ::std::result::Result<T, E>. I'm not sure if that applies in Dave's case too, but it seems like it might.

This extracts out the `SnakeCaseResult` type used to serialize /
deserialize a Rust `Result` in a way that satisfies our OpenAPI linting
rules, from a private crate location to a small helper crate. We can add
more helpers or tricks here as needed.
@bnaecker
Copy link
Collaborator Author

bnaecker commented May 8, 2025

Thanks for the tip @jgallagher. I've made us a small helper crate serde-helpers with that snake case workaround type. We can add more things as we need here. LMK if you have suggestions!

@bnaecker bnaecker requested review from ahl and jgallagher May 8, 2025 22:59
- Move to omicron-common
- Simplify serde with by grouping into module
- Add x-rust-type extension for client generators
@bnaecker bnaecker requested a review from ahl May 9, 2025 17:41
@bnaecker
Copy link
Collaborator Author

bnaecker commented May 9, 2025

@ahl I've reworked the JSONSchema implementation to include the Rust type extension stuff. Doesn't look like we can use only the #[serde(with = "...")] at the use sites, we do still need that and the schemars(with = "...") as well. But would love another look at some point.

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.

wicket screen should show present but faulted or unreadable transceivers
3 participants