-
Notifications
You must be signed in to change notification settings - Fork 84
Add system-version to omdb db #10481
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?
Changes from all commits
72260dd
23d8c9c
1746781
dd347a4
1422646
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| // This Source Code Form is subject to the terms of the Mozilla Public | ||
| // License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| // file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
|
||
| //! `omdb db target-release` subcommand | ||
| //! | ||
| //! Shows the current target release and date when update was requested. | ||
| //! When `--all` option is used, lists all target release records. | ||
|
|
||
| use crate::db::DbFetchOptions; | ||
| use crate::db::check_limit; | ||
| use crate::helpers::datetime_rfc3339_concise; | ||
| use anyhow::Context; | ||
| use async_bb8_diesel::AsyncRunQueryDsl; | ||
| use clap::Args; | ||
| use diesel::ExpressionMethods; | ||
| use diesel::QueryDsl; | ||
| use diesel::SelectableHelper; | ||
| use nexus_db_model::TargetRelease; | ||
| use nexus_db_model::TargetReleaseSource; | ||
| use nexus_db_queries::context::OpContext; | ||
| use nexus_db_queries::db::DataStore; | ||
| use nexus_db_schema::schema::target_release::dsl; | ||
| use tabled::Tabled; | ||
|
|
||
| #[derive(Debug, Args, Clone)] | ||
| pub(super) struct TargetReleaseArgs { | ||
| /// List the most recent target releases, sorted from oldest to newest | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's newest to oldest? |
||
| #[clap(long)] | ||
| all: bool, | ||
| } | ||
|
|
||
| #[derive(Tabled)] | ||
| #[tabled(rename_all = "SCREAMING_SNAKE_CASE")] | ||
| struct TargetReleaseRow { | ||
| target_release: String, | ||
| time_requested: String, | ||
| } | ||
|
|
||
| pub(super) async fn cmd_db_target_release( | ||
| opctx: &OpContext, | ||
| datastore: &DataStore, | ||
| fetch_opts: &DbFetchOptions, | ||
| args: &TargetReleaseArgs, | ||
| ) -> Result<(), anyhow::Error> { | ||
| let releases = if args.all { | ||
| let limit = fetch_opts.fetch_limit; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I would probably lean towards returning an error in case someone wishes to parse the output of this command.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are many, many
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, either way 🤷♀️ it's just for for clarity really |
||
| let conn = datastore.pool_connection_for_tests().await?; | ||
| let mut releases: Vec<TargetRelease> = dsl::target_release | ||
| .select(TargetRelease::as_select()) | ||
| .order_by(dsl::generation.desc()) | ||
| .limit(i64::from(u32::from(limit))) | ||
| .load_async(&*conn) | ||
| .await | ||
| .context("listing target releases")?; | ||
| check_limit(&releases, limit, || { | ||
| String::from("listing target releases") | ||
| }); | ||
| releases.reverse(); | ||
| releases | ||
| } else { | ||
| vec![ | ||
| datastore | ||
| .target_release_get_current(opctx) | ||
| .await | ||
| .context("fetching current target release")?, | ||
| ] | ||
| }; | ||
|
|
||
| let mut rows = Vec::with_capacity(releases.len()); | ||
| for release in releases { | ||
| let target_release = match release | ||
| .release_source() | ||
| .context("interpreting target release source")? | ||
| { | ||
| TargetReleaseSource::Unspecified => "<unspecified>".to_string(), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is printing rows from the db - what would you do instead of printing a sentinel like this? Neither skipping nor failing seems great to me.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking of skipping, but yeah, it would be useful to know when there have been unspecified target releases |
||
| TargetReleaseSource::SystemVersion(tuf_repo_id) => datastore | ||
| .tuf_repo_get_version(opctx, &tuf_repo_id) | ||
| .await | ||
| .with_context(|| format!("fetching TUF repo {tuf_repo_id}"))? | ||
| .to_string(), | ||
| }; | ||
| rows.push(TargetReleaseRow { | ||
| target_release, | ||
| time_requested: datetime_rfc3339_concise(&release.time_requested), | ||
| }); | ||
| } | ||
|
|
||
| let table = tabled::Table::new(rows) | ||
| .with(tabled::settings::Style::empty()) | ||
| .with(tabled::settings::Padding::new(1, 1, 0, 0)) | ||
| .to_string(); | ||
|
karencfv marked this conversation as resolved.
|
||
|
|
||
| println!("{}", table); | ||
|
|
||
| Ok(()) | ||
| } | ||
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.
Taking a closer look, we may want to change the functionality of the command a bit. Right now we only have two options right? All or one. What if the user wants to see just the latest 5?
In this scenario, it's probably a simpler user experience to have a command that always shows a list of target releases with a limit of records shown. Something like
omdb target-release listoromdb target-releaseswith an optionalfetch-limitor--limitflag. This way a user can say I want to see the latest one, or the last 10, or 100 or whatever.We already have this implemented for several endpoints (
DbFetchOptions), seecmd_db_blueprints,cmd_db_physical_disksetc. IfDbFetchOptionsdoesn't make sense for this command, you can also look at howcmd_reconfigurator_config_historyimplements a--limitflag.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.
We already have a
--fetch-limitflag onomdb dbthat applies to any subcommand that respects it (which this one does AFAICT, but it would be good to test / confirm).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.
I re-added the
DbFetchOptionsto respect this.