Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 74 additions & 1 deletion dev-tools/omdb/src/bin/omdb/db/ereport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,19 @@ struct ListArgs {
/// Include only ereports collected after this timestamp
#[clap(long, short)]
after: Option<DateTime<Utc>>,

/// Filter ereports based on whether or not their database records have
/// been marked as "seen" (included in a committed sitrep).
///
/// Note that ereports which have not been marked in the database *may*
/// still have been included in a sitrep. Marking of ereport database
/// records occurs in the background, and may not be up to date with the
/// latest sitrep. If an ereport has been marked as seen, it has
/// *definitely* been included in a committed sitrep, but if an ereport
/// has not been marked, it *may or may not* have been included in a
/// committed sitrep.
Comment on lines +93 to +99
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for including this note; it's true, and also an important caveat for anyone using this

#[clap(long = "seen", short = 'm', value_enum, default_value_t)]
seen: Seen,
}

#[derive(Debug, Args, Clone)]
Expand All @@ -99,6 +112,21 @@ struct ReportersArgs {
serial: Option<String>,
}

#[derive(
Copy, Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, clap::ValueEnum,
)]
enum Seen {
/// Include all ereports, regardless of whether or not they have been
/// marked.
#[default]
All,
/// Include only ereports whose database records have been marked as seen.
Marked,
/// Include only ereports whose database records have NOT been marked as
/// seen.
Unmarked,
}

pub(super) async fn cmd_db_ereport(
datastore: &DataStore,
fetch_opts: &DbFetchOptions,
Expand Down Expand Up @@ -138,6 +166,22 @@ async fn cmd_db_ereport_list(
serial: Option<&'report str>,
#[tabled(display_with = "display_option_blank", rename = "P/N")]
part_number: Option<&'report str>,

/// The underlying ereport record. This is not displayed when
/// formatting, but is retained so that the `EreportRow` can be
/// converted into an `EreportRowWithMark` if the markedness is needed.
#[tabled(skip)]
ereport: &'report db::model::Ereport,
}

/// Adds a `MARK` column to `EreportRow` to indicate whether or not the
/// ereport has been marked as seen in the database.
#[derive(Tabled)]
#[tabled(rename_all = "SCREAMING_SNAKE_CASE")]
struct EreportRowWithMark<'report> {
mark: &'static str,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: This could maybe be an enum with a "to string" impl instead of an arbitrary static str, right? but I don't care too much

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I went back and forth on this because I also didn't love the idea of an enum with a fmt::Display implementation that was an empty string. The reason I had left a similar suggestion on your PR was because we also had logic that was checking for a specific string. I think we could make this an enum but i don't think it's worth it in this case.

#[tabled(inline)]
ereport: EreportRow<'report>,
}

impl<'report> From<&'report db::model::Ereport> for EreportRow<'report> {
Expand Down Expand Up @@ -169,10 +213,27 @@ async fn cmd_db_ereport_list(
source,
serial: serial_number.as_deref(),
part_number: part_number.as_deref(),
ereport,
}
}
}

impl<'report> From<EreportRow<'report>> for EreportRowWithMark<'report> {
fn from(row: EreportRow<'report>) -> Self {
// The column is named "MARK" and its values are either "seen" or
// "", because I wanted to make it as obvious as possible that an
// unmarked ereport may still have been seen in a sitrep. The more
// obvious option of a "SEEN" column with values "yes" and "no" is
// potentially misleading, as "no" would suggest the ereport has not
// been seen, when actually it has not been *marked*. So, the column
// is named "MARK". But, I wanted to include the word "seen", so
// marked-as-seen ereports have the value "seen".
let mark =
if row.ereport.marked_seen_in.is_some() { "seen" } else { "" };
Self { mark, ereport: row }
}
}

if let (Some(before), Some(after)) = (args.before, args.after) {
anyhow::ensure!(
after < before,
Expand Down Expand Up @@ -213,6 +274,12 @@ async fn cmd_db_ereport_list(
query = query.filter(dsl::time_deleted.is_null());
}

match args.seen {
Seen::Marked => query = query.filter(dsl::marked_seen_in.is_not_null()),
Seen::Unmarked => query = query.filter(dsl::marked_seen_in.is_null()),
Seen::All => {}
}

let ereports = query.load_async(&*conn).await.with_context(ctx)?;
check_limit(&ereports, fetch_opts.fetch_limit, ctx);

Expand All @@ -229,7 +296,13 @@ async fn cmd_db_ereport_list(
)
});

let mut table = tabled::Table::new(rows);
let mut table = if args.seen == Seen::All {
// If both marked and unmarked ereports were included, add the "mark"
// column.
tabled::Table::new(rows.into_iter().map(EreportRowWithMark::from))
} else {
tabled::Table::new(rows)
};
table
.with(tabled::settings::Style::empty())
.with(tabled::settings::Padding::new(0, 1, 0, 0));
Expand Down
78 changes: 63 additions & 15 deletions dev-tools/omdb/tests/usage_errors.out
Original file line number Diff line number Diff line change
Expand Up @@ -562,27 +562,75 @@ List ereports
Usage: omdb db ereport list [OPTIONS]

Options:
--log-level <LOG_LEVEL> log level filter [env: LOG_LEVEL=] [default: warn]
-s, --serial <SERIALS> Include only ereports from systems with the provided serial numbers
-i, --id <IDS> Include only ereports from the provided reporter restart IDs
-c, --class <CLASSES> Include only ereports with the provided class strings
-b, --before <BEFORE> Include only ereports collected before this timestamp
--color <COLOR> Color output [default: auto] [possible values: auto, always, never]
-a, --after <AFTER> Include only ereports collected after this timestamp
-h, --help Print help
--log-level <LOG_LEVEL>
log level filter

[env: LOG_LEVEL=]
[default: warn]

-s, --serial <SERIALS>
Include only ereports from systems with the provided serial numbers

-i, --id <IDS>
Include only ereports from the provided reporter restart IDs

-c, --class <CLASSES>
Include only ereports with the provided class strings

-b, --before <BEFORE>
Include only ereports collected before this timestamp

--color <COLOR>
Color output

[default: auto]
[possible values: auto, always, never]

-a, --after <AFTER>
Include only ereports collected after this timestamp

-m, --seen <SEEN>
Filter ereports based on whether or not their database records have been marked as "seen"
(included in a committed sitrep).

Note that ereports which have not been marked in the database *may* still have been
included in a sitrep. Marking of ereport database records occurs in the background, and
may not be up to date with the latest sitrep. If an ereport has been marked as seen, it
has *definitely* been included in a committed sitrep, but if an ereport has not been
marked, it *may or may not* have been included in a committed sitrep.

Possible values:
- all: Include all ereports, regardless of whether or not they have been marked
- marked: Include only ereports whose database records have been marked as seen
- unmarked: Include only ereports whose database records have NOT been marked as seen

[default: all]

-h, --help
Print help (see a summary with '-h')

Connection Options:
--db-url <DB_URL> URL of the database SQL interface [env: OMDB_DB_URL=]
--dns-server <DNS_SERVER> [env: OMDB_DNS_SERVER=]
--db-url <DB_URL>
URL of the database SQL interface

[env: OMDB_DB_URL=]

--dns-server <DNS_SERVER>
[env: OMDB_DNS_SERVER=]

Database Options:
--fetch-limit <FETCH_LIMIT> limit to apply to queries that fetch rows [env:
OMDB_FETCH_LIMIT=] [default: 500]
--include-deleted whether to include soft-deleted records when enumerating objects
that can be soft-deleted
--fetch-limit <FETCH_LIMIT>
limit to apply to queries that fetch rows

[env: OMDB_FETCH_LIMIT=]
[default: 500]

--include-deleted
whether to include soft-deleted records when enumerating objects that can be soft-deleted

Safety Options:
-w, --destructive Allow potentially-destructive subcommands
-w, --destructive
Allow potentially-destructive subcommands
---------------------------------------------
stderr:
=============================================
Expand Down
Loading