add wizard to cscs login command

[Panaetius]

No description provided.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit b2eda4f)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Config Handling The logic for checking and setting the compute platform in the login flow could be simplified and made more robust by handling potential errors from get_available_compute_platforms more gracefully. if !source.1 && !source.2 { // don't override platform if it's already set if let Ok(available_platforms) = get_available_compute_platforms().await && !available_platforms.is_empty() && let Err(e) = config.set( "cscs.current_platform", available_platforms[0].to_string(), true, ) { error_tx .send(format!( "{:?}", Err::<(), Report>(e).wrap_err("Couldn't set currnt platform") )) .await .unwrap(); return; } } Wizard Flow The CLI wizard flow for selecting compute platform and account could benefit from better error handling and user experience improvements, especially when dealing with skipped inputs or unavailable platforms. // select compute platform let mut config = Config::new()?; let source = config.value_source("cscs.current_platform"); if !source.1 && !source.2 { let available_platforms: Vec<_> = get_available_compute_platforms() .await .unwrap_or(<ComputePlatform as VariantArray>::VARIANTS.to_vec()) .iter() .map(|c| c.to_string()) .collect(); let platform = Select::new("Compute Platform:", available_platforms).prompt()?; config.set("cscs.current_platform", platform, true)?; } // select cscs account let source = config.value_source("cscs.account"); if !source.1 && !source.2 && let Ok(Some(account)) = Text::new("CSCS Account:").prompt_skippable() && !account.is_empty() { config.set("cscs.account", account, true)?; } Platform Discovery The method for discovering available compute platforms makes multiple API calls which could be inefficient; consider optimizing this process or caching results. pub(crate) async fn get_available_compute_platforms() -> Result<Vec<ComputePlatform>> { match get_access_token().await { Ok(access_token) => { let mut platforms = Vec::new(); for platform in ComputePlatform::iter() { let api_client = CscsApi::new(access_token.0.clone(), Some(platform.clone())).unwrap(); if (api_client.list_systems().await).is_ok() { platforms.push(platform); } } Ok(platforms) } Err(e) => Err(e), } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to b2eda4f
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent panics from unwrapping API client creation The function uses unwrap() which can panic if CscsApi::new fails. This should be handled more gracefully to prevent crashes and provide better error reporting. coman/src/cscs/handlers.rs [93-107] pub(crate) async fn get_available_compute_platforms() -> Result<Vec<ComputePlatform>> { match get_access_token().await { Ok(access_token) => { let mut platforms = Vec::new(); for platform in ComputePlatform::iter() { - let api_client = CscsApi::new(access_token.0.clone(), Some(platform.clone())).unwrap(); - if (api_client.list_systems().await).is_ok() { - platforms.push(platform); + match CscsApi::new(access_token.0.clone(), Some(platform.clone())) { + Ok(api_client) => { + if (api_client.list_systems().await).is_ok() { + platforms.push(platform); + } + } + Err(_) => continue, // Skip platforms that fail to initialize } } Ok(platforms) } Err(e) => Err(e), } } Suggestion importance[1-10]: 9 __ Why: This suggestion addresses a critical issue where unwrap() could cause a panic if CscsApi::new fails. The improved code uses match to handle the error gracefully, continuing to check other platforms instead of crashing. This is a high-impact fix for robustness and reliability.	High
	Handle potential errors when fetching available platforms The code assumes that get_available_compute_platforms() will always succeed, but it can fail. This could cause the application to crash or behave unexpectedly. It should handle potential errors gracefully. coman/src/cscs/cli.rs [44-58] // select compute platform let mut config = Config::new()?; let source = config.value_source("cscs.current_platform"); if !source.1 && !source.2 { - let available_platforms: Vec<_> = get_available_compute_platforms() - .await - .unwrap_or(<ComputePlatform as VariantArray>::VARIANTS.to_vec()) - .iter() - .map(|c| c.to_string()) - .collect(); + let available_platforms = match get_available_compute_platforms().await { + Ok(platforms) => platforms.iter().map(|c| c.to_string()).collect(), + Err(_) => <ComputePlatform as VariantArray>::VARIANTS.to_vec().iter().map(|c| c.to_string()).collect(), + }; let platform = Select::new("Compute Platform:", available_platforms).prompt()?; config.set("cscs.current_platform", platform, true)?; } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential issue where get_available_compute_platforms() can fail and cause the application to crash. The improved code properly handles this by using match to gracefully fall back to default variants if the fetch fails, which is a significant improvement over the existing code that unconditionally calls unwrap_or.	Medium
	Improve error handling for platform selection The code does not handle the case where get_available_compute_platforms() returns an error, which could lead to unexpected behavior. Additionally, the logic for setting the platform can be simplified by using unwrap_or_else for cleaner error handling. coman/src/app/model.rs [513-549] Ok(_) => { let mut config = match Config::new() { Ok(config) => config, Err(e) => { error_tx .send(format!( "{:?}", Err::<(), Report>(e).wrap_err("Couldn't create config object") )) .await .unwrap(); return; } }; let source = config.value_source("cscs.current_platform"); if !source.1 && !source.2 { // don't override platform if it's already set - if let Ok(available_platforms) = get_available_compute_platforms().await - && !available_platforms.is_empty() - && let Err(e) = config.set( + let available_platforms = get_available_compute_platforms().await.unwrap_or_else(|_| vec![]); + if !available_platforms.is_empty() { + if let Err(e) = config.set( "cscs.current_platform", available_platforms[0].to_string(), true, - ) - { - error_tx - .send(format!( - "{:?}", - Err::<(), Report>(e).wrap_err("Couldn't set currnt platform") - )) - .await - .unwrap(); - return; + ) { + error_tx + .send(format!( + "{:?}", + Err::<(), Report>(e).wrap_err("Couldn't set current platform") + )) + .await + .unwrap(); + return; + } } } event_tx.send(UserEvent::Cscs(CscsEvent::LoggedIn)).await.unwrap() } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the code doesn't handle errors from get_available_compute_platforms() and proposes a cleaner approach using unwrap_or_else. However, the improvement doesn't significantly change the behavior since the original code already handles errors via unwrap_or_default(), and the suggestion's approach still relies on unwrap_or_else which could also panic if the fallback fails.	Medium

---

Previous suggestions

Suggestions up to commit ab66759

Category	Suggestion	Impact
Possible issue	Handle errors when fetching available platforms The code does not handle the case where get_available_compute_platforms() returns an error, potentially leading to unexpected behavior when no platforms are available. Consider propagating this error instead of silently continuing. coman/src/app/model.rs [513-548] Ok(_) => { let mut config = match Config::new() { Ok(config) => config, Err(e) => { error_tx .send(format!( "{:?}", Err::<(), Report>(e).wrap_err("Couldn't create config object") )) .await .unwrap(); return; } }; let source = config.value_source("cscs.current_platform"); if !source.1 && !source.2 { // don't override platform if it's already set - if let Ok(available_platforms) = get_available_compute_platforms().await - && !available_platforms.is_empty() - && let Err(e) = config.set( - "cscs.current_platform", - available_platforms[0].to_string(), - true, - ) { + match get_available_compute_platforms().await { + Ok(available_platforms) if !available_platforms.is_empty() => { + if let Err(e) = config.set( + "cscs.current_platform", + available_platforms[0].to_string(), + true, + ) { + error_tx + .send(format!( + "{:?}", + Err::<(), Report>(e).wrap_err("Couldn't set current platform") + )) + .await + .unwrap(); + return; + } + } + Ok(_) => { + // No platforms available, continue without setting + } + Err(e) => { error_tx .send(format!( "{:?}", - Err::<(), Report>(e).wrap_err("Couldn't set currnt platform") + Err::<(), Report>(e).wrap_err("Failed to fetch available platforms") )) .await .unwrap(); return; } + } } event_tx.send(UserEvent::Cscs(CscsEvent::LoggedIn)).await.unwrap() } Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a potential issue where get_available_compute_platforms() could fail but the code previously ignored such failures. The improved code properly handles the error case by propagating it, which enhances robustness and prevents silent failures.	Medium
	Propagate errors from platform fetching The code uses unwrap_or which can lead to unexpected behavior if get_available_compute_platforms() fails. It should propagate the error instead of defaulting to all variants. coman/src/cscs/cli.rs [44-58] // select compute platform let mut config = Config::new()?; let source = config.value_source("cscs.current_platform"); if !source.1 && !source.2 { - let available_platforms: Vec<_> = get_available_compute_platforms() + let available_platforms = get_available_compute_platforms() .await - .unwrap_or(<ComputePlatform as VariantArray>::VARIANTS.to_vec()) + .map_err(|e| eyre!("Failed to fetch available platforms: {:?}", e))? .iter() .map(|c| c.to_string()) - .collect(); + .collect::<Vec<_>>(); let platform = Select::new("Compute Platform:", available_platforms).prompt()?; config.set("cscs.current_platform", platform, true)?; } Suggestion importance[1-10]: 7 __ Why: The suggestion accurately points out the problematic use of unwrap_or which could hide errors. The improved code correctly propagates errors using map_err, making the behavior more predictable and robust.	Medium
General	Reintroduce automatic platform selection The function cscs_login no longer sets the compute platform automatically, which may cause issues for users who rely on this behavior. Consider reintroducing logic to detect and set the appropriate platform. coman/src/cscs/handlers.rs [84-92] pub(crate) async fn cscs_login(client_id: String, client_secret: String) -> Result<()> { let client_id_secret = Secret::new(client_id); store_secret(CLIENT_ID_SECRET_NAME, client_id_secret.clone()).await?; let client_secret_secret = Secret::new(client_secret); store_secret(CLIENT_SECRET_SECRET_NAME, client_secret_secret.clone()).await?; client_credentials_login(client_id_secret, client_secret_secret) .await - .map(|_| ()) + .map(|_| ())?; + + // Reintroduce logic to set compute platform if needed + let mut config = Config::new()?; + let source = config.value_source("cscs.current_platform"); + if !source.1 && !source.2 { + // Set first available platform + if let Ok(available_platforms) = get_available_compute_platforms().await + && !available_platforms.is_empty() { + config.set("cscs.current_platform", available_platforms[0].to_string(), true)?; + } + } + Ok(()) } Suggestion importance[1-10]: 6 __ Why: This suggestion addresses a functional change in behavior where automatic platform selection was removed. While the suggestion provides a reasonable approach to reintroduce this feature, it's less critical than the previous two suggestions because the core functionality remains intact and the change was likely intentional.	Low

[github-actions]

Persistent review updated to latest commit b2eda4f