feat(CLI): Agama profile import command#1270
Conversation
The documentation for Agama's CLI is rather poor, making it hard to use. The goal of this pull request is to improve such documentation by: * Expanding the existing descriptions. * Unifying the style to make it more consistent. * Adding the missing documentation. <details> <summary>agama auth --help</summary>  </details> ## Out of scope * This PR does not adapt the `profile` command because we are actively working on it (see #1270). * I did not update the changelog. We will do it once https://trello.com/c/V6ML7csV/3686-5-improve-the-cli is finished.
imobachgs
left a comment
imobachgs
left a comment
There was a problem hiding this comment.
Just a few comments. I have the impression that splitting the import function into smaller steps would make things easier follow.
rust/agama-cli/src/profile.rs
Outdated
| /// Evaluate a profile, injecting the hardware information from D-Bus | ||
| Evaluate { path: String }, | ||
|
|
||
| /// Import autoinstallation profile from given location into agama configuration |
There was a problem hiding this comment.
This first line will be shown as a short summary of the list of commands so perhaps you can use something shorter and explain it below.
After all, from a user's perspective, what "importing a profile into Agama configuration" means?
| } | ||
|
|
||
| pub fn run(subcommand: ProfileCommands) -> anyhow::Result<()> { | ||
| async fn import(url: String, dir: Option<PathBuf>) -> anyhow::Result<()> { |
There was a problem hiding this comment.
The other commands (download and evaluate) rely on a separate struct. If you do not want to build a struct for this, it is fine, but at least I would move the logic to the same separate module.
I would like to keep the functions here to handle the details that are specific to the commands (setting default values, etc.).
rust/agama-cli/src/profile.rs
Outdated
| let output_path_str = output_path.to_str().context("Failed to get output path")?.to_string(); | ||
| // TODO: optional skip of validation | ||
| validate(output_path_str.clone())?; | ||
| crate::config::run(crate::config::ConfigCommands::Load { path: output_path_str }, crate::printers::Format::Json).await?; |
There was a problem hiding this comment.
Oh, it looks like we're calling the same binary again with different options. I'm not sure I like this approach.
Perhaps we might think about building a Config API that can be used in config::run. Let's keep it by now but, please, add a FIXME.
There was a problem hiding this comment.
in fact it is not same binary...I just call method that implements config load. Otherwise I will call just agama config load via Command, but I think this will be faster?
There was a problem hiding this comment.
OK, not literally a binary, but it looks weird anyway; you even have to pass the format to use.
There was a problem hiding this comment.
yeah, reason is that config load has it inside this run command - https://github.com/openSUSE/agama/blob/master/rust/agama-cli/src/config.rs#L89
There was a problem hiding this comment.
Yes, I know the reason :-) and I dislike it. I will add this to the list of things to fix in the Rust codebase.
rust/agama-cli/src/profile.rs
Outdated
| let err = Command::new("bash") | ||
| .args([output_path.to_str().context("Wrong path to shell script")?]) | ||
| .exec(); | ||
| println!("Error: {}", err); |
There was a problem hiding this comment.
So it will always print this string, right?
There was a problem hiding this comment.
no. It is printed only if exec() failed. Exec replace current process with the new one...and only if it failed, it continues with execution.
There was a problem hiding this comment.
Ah, ok, I understand now. Then let's just eprintln! and that's it.
rust/agama-cli/src/profile.rs
Outdated
| output_path = output_dir.join("profile.json"); | ||
| } | ||
|
|
||
| let output_path_str = output_path |
There was a problem hiding this comment.
I would expect something with the _str suffix to be of type &str 😉
Co-authored-by: Imobach González Sosa <igonzalezsosa@suse.com>
jreidinger
changed the title
Prepare for releasing Agama 9. It includes the following pull requests: - #1101 - #1202 - #1228 - #1231 - #1236 - #1238 - #1239 - #1240 - #1242 - #1243 - #1244 - #1245 - #1246 - #1247 - #1248 - #1249 - #1250 - #1251 - #1252 - #1253 - #1254 - #1255 - #1256 - #1257 - #1258 - #1259 - #1260 - #1261 - #1264 - #1265 - #1267 - #1268 - #1269 - #1270 - #1271 - #1272 - #1273 - #1274 - #1279 - #1280 - #1284 - #1285 - #1286 - #1287 - #1288 - #1289 - #1290 - #1291 - #1292 - #1293 - #1294 - #1295 - #1296 - #1298 - #1299 - #1300 - #1301 - #1302 - #1303 - #1304 - #1305 - #1306 - #1307 - #1308 - #1309 - #1310 - #1311 - #1312 - #1313 - #1314 - #1315 - #1316 - #1317 - #1318 - #1319 - #1320 - #1321 - #1322 - #1323 - #1324 - #1325 - #1326 - #1328 - #1329 - #1331 - #1332 - #1334 - #1338 - #1340 - #1341 - #1342 - #1343 - #1344 - #1345 - #1348 - #1349 - #1351 - #1352 - #1353 - #1354 - #1355 - #1356 - #1357 - #1358 - #1359 - #1360 - #1361 - #1362 - #1363 - #1365 - #1366 - #1367 - #1368 - #1371 - #1372 - #1374 - #1375 - #1376 - #1379 - #1380 - #1381 - #1383 - #1384 - #1385 - #1386 - #1387 - #1388 - #1389 - #1391 - #1392 - #1394 - #1395 - #1397 - #1398 - #1399 - #1400 - #1403
3 participants
Problem
There are many steps in autoinstallation processing that is splitted over multiple command line arguments. It is problematic if user use shell script based autoinstallation and need to all of those steps.
Solution
Introduce new "agama profile import" command that do all autoinstallation processing. Only install part is not done. This allows user to use hooks or tuning before real installation begins.