I believe KeyDatabase name is the thing of the past which was made to
simplify migration to Sqlite without doing the renaming all over code
base (which this commit does). So, this is a bit of technical debt IMO
and direct usage of Sqlite would serve better code readability.
KeyDatabase leaks presence of Sqlite though functions like new_file() and
others as such it doesn't serve very good the purpose of abstracting
things away and hiding the fact of Sqlite behind.
Code review issue: https://gitlab.com/keys.openpgp.org/hagrid/-/merge_requests/214#note_2484117659
Changes:
- Extract database file path and log dir path constructions from
Sqlite::new_file() and Sqlite::new_internal() associated functions to
Sqlite::db_file_path() and Sqlite::log_dir_path().
These path manipulations inside Sqlite/KeyDatabase wasn't ideal as
they increased diversity of reasons why Sqlite constructions can fail,
unnecessary constrain how Sqlite and with which paths can be used, and
that logic doesn't actually belong to Sqlite itself. So, later we
probably have to move these db_file_path() and log_dir_path()
functions else where.
- Replace base_dir parameter of Sqlite::new_file() with "db_file" and "log_dir"
params which gives us a way to use database file passed in command
line options.
- Add Sqlite::log_dir_path_from_db_file_path() associated function to
infer log_dir_path based on given db_file_path.
It is used in command handler where only db_file_path provided.
- Pull up log dir construction from Sqlite::new_internal() to
Sqlite::new_file() to avoid passing through log_dir_path through
several functions. This also makes Sqlite::new_file() signature
cleaner.
- Fix usages of Sqlite/KeyDatabase::new_file through out the code base:
and instead of providing base_dir, populate db_file_path and
log_dir_path using Sqlite::db_file_path() and Sqlite::log_dir_path()
functions which calculate paths based on provided base_dir.
- Remove "-c, --config" and "-e, --environment" options from
hagridctl::cli::Cli.
- Remove hagridctl::cli::DeploymentEnvironment enum.
- Remove entire hagridctl::config module as we don't extract paths from
configuration any more.
- Add "env" feature flag to "clap" dependency in hagridctl/Cargo.toml to
be able to populate Cli::db_file_path field from HAGRID_DB_FILE_PATH
environment variable.
- Add optional --db-file-path option to "hagridctl". Cli::db_file_path
is obligatory, but when --db-file-path is not provided Clap tries to
populate it from HAGRID_DB_FILE_PATH env var and if that's not
possible Clap terminates with error message.
- In hagridctl::cli::dispatch_cmd() use db_file_path from
cli.db_file_path instead of keys_internal_dir extracted from configuration.
- Replace keys_db_internal_dir with db_file_path in the following
command handlers:
- hagridctl::delete::run
- hagridctl::import::run
- Get rid of unnecessary Result unwrapping and wrapping it again in
hagrid::web::configure_db_service function and return the value from
KeyDatabase::new_file immediately instead.
Code review issue: https://gitlab.com/keys.openpgp.org/hagrid/-/merge_requests/214#note_2482983036
Changes:
- Change type of "base_dir" argument of database::Sqlite::new_file()
associated function form "impl Into<PathBuf>" to "impl AsRef<Path>"
as AsRef<Path> is more generic.
- Remove "base" argument from "hagridctl delete" command.
- Extract "config" and "environment" options from "import" command to
hagridctl::cli::Cli struct.
- Make "config" and "environment" options global, i.e. they can be
specified with any command.
- Change "environment" from being argument to option as being argument
doesn't play well with "import" command where arbitrary list of
KEYRING_FILES can be specified. In general, being global works much
better with options then arguments.
So, <ENVIRONMENT> argument became "-e, --environment <ENVIRONMENT>" option
with the same default value (production).
- KEYRING_FILES in "import" command don't need to be "last" any more.
- Get "keys_db_internal_dir" in hagridctl::cli::dispatch_cmd() and let
it be propagated to places where KeyDatabase is created instead of
progapagation of config or base_dir.
Before merging "hagrid-delete" into "hagridctl" the later was
exclusively used with "import" command. So, it doesn't make a difference
how "config" and "ENVIRONMENT" parameters were specified. After merge
these arguments doesn't make sense at the top level any more, hence the
move.
Changes:
- Move "file" and "env" field from hagridctl::cli::Cli struct into
hagridctl::cli::Command::Import enum variant.
- Because of the move and optionality of "ENVIRONMENT" argument for
"import" command I had to add "last = true" option to KEYRING_FILES
argument which requires their specification to be prepended with --.
- Clean up hagridctl::cli::dispatch_cmd() function as match statement
now can take ownership over cli.command and "file" and "env" fields of
Command::Import can be destructured and passed directly to
config::load() instead of passing around reference to Cli struct.
Overall code becomes cleaner, IMO.
- Change config::load() to take "file" and "env" directly instead of
"cli" parameter.
- Get rid of run() function in hagridctl/src/main.rs as it becomes
redundant.
Code review issue: https://gitlab.com/keys.openpgp.org/hagrid/-/merge_requests/214#note_2482960066
Changes:
- Remove "hagrid-delete" bin declaration from Cargo.toml.
- Remove "clap" dependency for "hagrid" crate in Cargo.toml as after
moving "hagrid-delete" to "hagridctl" "hagrid" crate does not use
"clap" any more.
- Remove "run-hagrid-delete" recipe with its "hagrid-delete" and
"delete" aliases from justfile.
- Update Cargo.lock.
- Create "delete" command for "hagridctl" by adding cli::Command::Delete
variant which previously were hagrid-delete::cli::Cli struct.
- Move "delete" module with command handler ("run" function) from
"hagrid" crate to hagridctl::delete.
- Extend hagridctl::cli::dispatch_cmd() function with processing of
cli::Command::Delete variant and call to hagridctl::delete::run.
- Move print_errors() from hagrid::delete::cli module to hagrdictl::cli.
- Move KeyDatabase instantiation from hagrid/src/main.rs into
hagridctl::delete::run command handler as this way of database
instantiation is specific to "delete" command.
Probably later we have to reconsider how we instantiate database for
all the commands to avoid reimplementing that functionality every time
and duplicating the code.
That move caused change in signature of hagridctl::delete::run
function. Now we pass base path instead of db reference.
Commands:
just lint
Warnings:
warning: this expression creates a reference which is immediately dereferenced by the compiler
--> hagridctl/src/cli.rs:39:58
|
39 | Command::Import { keyring_files } => import::run(&config, keyring_files.to_owned()),
| ^^^^^^^ help: change this to: `config`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
= note: `#[warn(clippy::needless_borrow)]` on by default
I think it looks a bit weird to have things like module "import" with function
"do_import" which is a command handler. So, I renamed all such command
handlers to simple "run" functions. IMO on call side it looks now
better, e.g.: import::run().
Changes:
- Rename the following command handler functions:
- hagridctl::import::do_import -> hagridctl::import::run
- hagrid::delete::delete:delete -> hagrid::delete::delete::run
(still weird, but minus one delete. First delete in path is bin
name, second - module for command handler)
- tester::generate::do_generate -> tester::generate::run
- tester::genreqs::do_genreqs -> tester::genreqs::run
Changes:
- Add "derive" feature for "clap" in hagridctl/Cargo.toml.
- Move about description from hagridctl/src/cli.rs to "description" field in
hagridctl/Cargo.toml. So, "clap" can access it via #[command(about)]
macro configuration.
- Update Cargo.lock.
- Replace in hagridctl/src/cli.rs usage of clap::App from Clap 2.34.0
with clap::Parser derive macro based API, cli::Cli struct and cli::Command
enum.
- Omit explicit specification of args name which resulted in upper
casing cmd option's parameters, e.g. "<keyring files>" -> "<KEYRING_FILES>".
- Replace in cli::dispatch_cmd() "if let" based logic with simple match
on cli::Command enum variants.
- Get rid of manual parsing of options' and commands' values in
cli::dispatch_cmd(). As it is hadled by derived
value_parser (https://docs.rs/clap/latest/clap/_derive/index.html#arg-attributes)
based on field types.
- Replace set of limited values specified with .possible_values() method
call with #[arg(value_enum)] macro and hagridctl::cli::DeploymentEnvironment
enum. I make environment names fully qualified, but left abbreviations
as aliases, so `just hagridctl dev import` kind of commands still
allowed.
- Replace in hagridctl/src/main.rs cli::app().get_matches() (which is Clap
v2 API) with cli::Cli::parse() (Clap v4 API).
- Add "unicode" feature flag for clap dependency in hagridctl/Cargo.toml.
- Refactor config::load() and to use cli::Cli struct instead of
clap::ArgMatches. Defaulting to Rocket.toml for configuration file was
moved to default value for hagridctl::cli::Cli.env field.
- Replace .unwrap() calls in config::load() function with ? operator.
- Extract match statement from config::load() to
HagridConfigs::get_by(deployment_environment) method.
Changes:
- Move the following structs from main.rs to "config" module:
- HagridConfigs
- HagridConfig
- Introduce "config::load" function and move configuration loading code into it.
- Adjust imports accordingly.
- Share ArgMatches returned by App::get_matches between
config::load and cli::dispatch_cmd functions.
- Make HagridConfigs struct private to the config module as rest of the
system doesn't need to know about it and interacts only with
HagridConfig struct.
Result it widely used type and the name is highly overloaded as almost
every library tend to define they own Result type.
When default Result type deviates from default it is usually harder to
read code and requires a bit of investigation work to understand this
particular Result corresponds to.
In Hagrid code base in many places anyhow::Result was made a default
type. It usually was imported through indirection, i.e.
use anyhow::Result;
was imported in files like
- database/src/lib.rs
- hagridctl/src/import.rs
- hagridctl/src/main.rs
- src/main.rs
and in submodules that above import was reimported with something like
use crate::Result;
or
use super::Result;
So, I qualified such Result as anyhow::Result which IMO make code easier
to understand and "anyhow" is short enough name to use along with
Result.
All imports and reimports was cleaned up.
Changes:
- Full qualify anyhow's Result name as anyhow::Result in places where it
is used to make code easier to read and understand.
- Remove imports and reimports of anyhow::Result.
- Restore default Result type to core::result::Result (aka std::result::Result).
- Clean up unnecesary name path qualification for Result type
like "std::result" or "result" as it becomes a default one and doesn't
overriden any more.
Both "extern crate" and #[macro_use] are artifacts of earlier editions
of Rust language. Nowadays we can entirely rely on "use" instead.
Changes:
- Replace "extern crate" with "use" imports.
- Replace
#[macro_use]
extern crate ...;
declarations with "use" imports of used macros. For example,
#[macro_use]
extern crate anyhow;
was replaced with
use anyhow::anyhow;
in every file where anyhow! macro were used.
- Favor direct usage of import path instead of aliased one.
For example, in many places "sequoia_opengpg" were aliased as "openpgp",
during imports replacements I tried to avoid usage of "openpgp" or
introduced additional aliases (like "use sequoia_openpgp as openpgp")
and used "sequoia_opengpg".
I think this way it is easier to understand where name came from
instead of search and jumping to lib.rs or main.rs files trying to
find where name were aliased.
Another example of such favoring is usage of "hagrid_database" over
the "database" in imports.
NOTE: the usage is still inconsistent and requires further clean up.
Commands:
cargo clippy --tests --no-deps --workspace
Warnings:
warning: the borrowed expression implements the required traits
--> database/src/fs.rs:258:40
|
258 | let typ = fs::symlink_metadata(&path).ok()?.file_type();
| ^^^^^ help: change this to: `path`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
= note: `#[warn(clippy::needless_borrows_for_generic_args)]` on by default
warning: the borrowed expression implements the required traits
--> database/src/fs.rs:287:44
|
287 | let typ = fs::symlink_metadata(&path)?.file_type();
| ^^^^^ help: change this to: `path`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
warning: the borrowed expression implements the required traits
--> database/src/fs.rs:328:13
|
328 | symlink(&symlink_content, &symlink_name_tmp)?;
| ^^^^^^^^^^^^^^^^ help: change this to: `symlink_content`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
warning: the borrowed expression implements the required traits
--> database/src/fs.rs:329:31
|
329 | rename(&symlink_name_tmp, &symlink_name)?;
| ^^^^^^^^^^^^^ help: change this to: `symlink_name`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
warning: the borrowed expression implements the required traits
--> database/src/fs.rs:334:35
|
334 | if let Ok(target) = read_link(&link) {
| ^^^^^ help: change this to: `link`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
warning: this expression creates a reference which is immediately dereferenced by the compiler
--> database/src/sqlite.rs:424:46
|
424 | .map(|email| Email::from_str(&email))
| ^^^^^^ help: change this to: `email`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
= note: `#[warn(clippy::needless_borrow)]` on by default
warning: this expression creates a reference which is immediately dereferenced by the compiler
--> database/src/lib.rs:546:51
|
546 | self.set_email_unpublished_filter(&tx, ¤t_fpr, |uid| {
| ^^^ help: change this to: `tx`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
warning: this expression creates a reference which is immediately dereferenced by the compiler
--> database/src/lib.rs:602:29
|
602 | self.regenerate_wkd(&tx, fpr_primary, &published_tpk_clean)?;
| ^^^ help: change this to: `tx`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
warning: this expression creates a reference which is immediately dereferenced by the compiler
--> hagridctl/src/import.rs:161:86
|
161 | db.set_email_published(&key_fpr.clone().try_into().unwrap(), &email)
| ^^^^^^ help: change this to: `email`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
= note: `#[warn(clippy::needless_borrow)]` on by default
warning: this expression creates a reference which is immediately dereferenced by the compiler
--> src/sealed_state.rs:20:37
|
20 | let cipher = Aes256Gcm::new(&key);
| ^^^^ help: change this to: `key`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
= note: `#[warn(clippy::needless_borrow)]` on by default
warning: the borrowed expression implements the required traits
--> src/template_helpers.rs:46:77
|
46 | remove_extension(remove_extension(path.strip_prefix(&template_path)?));
| ^^^^^^^^^^^^^^ help: change this to: `template_path`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
= note: `#[warn(clippy::needless_borrows_for_generic_args)]` on by default
warning: the borrowed expression implements the required traits
--> src/web/vks.rs:216:46
|
216 | Err(e) => return UploadResponse::err(&e.to_string()),
| ^^^^^^^^^^^^^^ help: change this to: `e.to_string()`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
warning: the borrowed expression implements the required traits
--> src/web/vks.rs:248:40
|
248 | return UploadResponse::err(&format!("error sending email to {}", &email));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `format!("error sending email to {}", &email)`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
It eliminates the following output of `cargo autoinherit` command:
`sequoia-openpgp` won't be auto-inherited because there are multiple sources for it:
- version: =1.17.0
- version: ^1.17.0
Changes:
- Allow "sequoia-openpgp" crate to use workspace dependencies by resolving
version differences constraints in Cargo.toml files.
It eliminates the following output of `cargo autoinherit` command:
`multipart` won't be auto-inherited because there are multiple sources for it:
- version: ^0
- version: ~0.18
Changes:
- Allow "multipart" crate to use workspace dependencies by resolving
version differences constraints in Cargo.toml files.
This simplifies dependencies management and upgrades while ensuring that
dependencies version aligned with all the crates in the project and
neither dependency is used twice with different versions by accident
(though dependencies still can appear several times as sub-dependencies due to
misaligned version constraints for dependency resolution).
Documentation and useful articles:
- https://mainmatter.com/blog/2024/03/18/cargo-autoinherit/
- https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#inheriting-a-dependency-from-a-workspace
- https://crates.io/crates/cargo-autoinherit
Commands:
`cargo autoinherit`
Output:
$ cargo autoinherit
`multipart` won't be auto-inherited because there are multiple sources for it:
- version: ^0.18.0
- version: ^0
`sequoia-openpgp` won't be auto-inherited because there are multiple sources for it:
- version: ^1.17.0
- version: =1.17.0
Changes:
- Collect all the dependencies for workspace's crates in the top level
Cargo.toml file by applying `cargo autoinherit`.
- Use workspace dependencies in crates Cargo.toml files (i.e.
crate_name = { workspace = true }).
Changes:
- Change edition in the following Cargo.toml files to 2024:
- Cargo.toml change edition: 2018 -> 2024
- Explicitly set 2024 (i.e. default 2015 -> 2024) edition
in the following files:
- database/Cargo.toml
- hagridctl/Cargo.toml
- tester/Cargo.toml
NOTE: setting explicitly edition also clean up WARNINGS like ones bellow:
warning: .../hagrid/database/Cargo.toml: no edition set: defaulting to the 2015 edition while the latest is 2024
warning: .../hagrid/tester/Cargo.toml: no edition set: defaulting to the 2015 edition while the latest is 2024
warning: .../hagrid/hagridctl/Cargo.toml: no edition set: defaulting to the 2015 edition while the latest is 2024
Starting with 1.18.0, the retain_userids method starts working
differently, returning an empty cert if no signed user ids or direct key
signature is left. Since we need this, we'll stay on 1.17.0 for now.
