add periodic self update

[Panaetius]

No description provided.

[github-actions]

PR Reviewer Guide 🔍

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 Update Logic The update check logic may not handle all edge cases for timestamp parsing or filesystem operations gracefully, potentially leading to silent failures. pub async fn check_update() -> () { let Ok(config) = Config::new() else { return; }; let data_dir = get_data_dir(); let stamp_path = data_dir.join("selfupdate.stamp"); let Ok(update_stamp) = std::fs::read_to_string(&stamp_path) else { std::fs::write(&stamp_path, Local::now().to_rfc3339()).unwrap(); return; }; let Ok(update_stamp) = DateTime::parse_from_rfc3339(&update_stamp) else { return; }; let now = Local::now(); if now.naive_local() - update_stamp.naive_local() > TimeDelta::hours(config.values.update_check_interval as i64) { println!("checking for updates"); let _ = update(); std::fs::write(&stamp_path, Local::now().to_rfc3339()).unwrap(); } } Configuration Default The new update_check_interval field in ComanConfig uses a default value of 0, which might lead to unexpected behavior if not explicitly configured. pub update_check_interval: u64, #[serde(default)]

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Improve error handling in update checking logic The check_update function has a potential panic risk when writing to the stamp file due to unwrapping errors. Additionally, the function does not handle errors during the update process gracefully, which could lead to silent failures. Consider using proper error handling instead of unwrap() and propagate errors appropriately. coman/src/cli/app.rs [592-612] -pub async fn check_update() -> () { +pub async fn check_update() { let Ok(config) = Config::new() else { return; }; let data_dir = get_data_dir(); let stamp_path = data_dir.join("selfupdate.stamp"); let Ok(update_stamp) = std::fs::read_to_string(&stamp_path) else { - std::fs::write(&stamp_path, Local::now().to_rfc3339()).unwrap(); + if let Err(e) = std::fs::write(&stamp_path, Local::now().to_rfc3339()) { + eprintln!("Failed to write update stamp: {}", e); + } return; }; let Ok(update_stamp) = DateTime::parse_from_rfc3339(&update_stamp) else { return; }; let now = Local::now(); if now.naive_local() - update_stamp.naive_local() > TimeDelta::hours(config.values.update_check_interval as i64) { println!("checking for updates"); - let _ = update(); - std::fs::write(&stamp_path, Local::now().to_rfc3339()).unwrap(); + if let Err(e) = update() { + eprintln!("Update failed: {}", e); + } + if let Err(e) = std::fs::write(&stamp_path, Local::now().to_rfc3339()) { + eprintln!("Failed to update stamp file: {}", e); + } } } Suggestion importance[1-10]: 7 __ Why: The suggestion identifies a potential panic risk from using unwrap() and proposes better error handling with eprintln! and proper error propagation, which improves robustness. However, it doesn't address the core issue of the function returning () instead of Result<()>, which is a more fundamental problem.	Medium
	Await async update check properly The call to check_update().await; in main doesn't await its result, potentially leading to unhandled errors. Since check_update is an async function, it should be awaited properly to ensure any errors during the update check are handled correctly. coman/src/main.rs [68] +// check self-update +check_update().await; - Suggestion importance[1-10]: 3 __ Why: The suggestion points out that check_update().await; doesn't await its result, but in Rust, calling an async function without awaiting it is a compile-time warning rather than a runtime error. This is a low-impact observation since the code still works as intended, just with a warning.	Low
General	Use environment variable for version retrieval The update function uses cargo_crate_version!() macro which may cause issues if the crate version isn't properly defined or accessible. It's better to use a more robust way to retrieve the version, such as fetching it from environment variables or a configuration file, to ensure consistent behavior across different build environments. coman/src/cli/app.rs [574-590] pub fn update() -> Result<()> { + let version = env!("CARGO_PKG_VERSION"); let status = self_update::backends::github::Update::configure() .repo_owner("SwissDataScienceCenter") .repo_name("coman") .bin_name("coman") .bin_path_in_archive("coman") .show_download_progress(true) - .current_version(cargo_crate_version!()) + .current_version(version) .build()? .update()?; if status.updated() { println!("Successfully updated to version: `{}`", status.version()); } else { println!("Already up to date at version: `{}`", status.version()); } Ok(()) } Suggestion importance[1-10]: 5 __ Why: The suggestion suggests using env!("CARGO_PKG_VERSION") instead of cargo_crate_version!() for more robust version retrieval. While this is a reasonable improvement, it's a minor enhancement and doesn't address any critical issues in the current implementation.	Low

[github-actions]

Version bump from 0.8.7 to 0.8.8 should be documented in changelog or release notes.

#ai-review-inline

[github-actions]

Missing default value for update_check_interval; this will cause a panic if not explicitly set in config.

Suggested change

	pub update_check_interval: u64,
	pub update_check_interval: u64 = 3600;

#ai-review-inline

[github-actions]

Added 'update' function to CLI commands, ensure it's properly implemented and tested.

Suggested change

	check_update, get_config, print_completions, set_config, update, version,
	check_update, get_config, print_completions, set_config, update, version,

#ai-review-inline

[github-actions]

The check_update call should handle errors gracefully to avoid silent failures.

Suggested change

	check_update().await;
	check_update().await;

#ai-review-inline

[github-actions]

The comment references external documentation; consider adding a brief explanation of what 'current_system' controls.

Suggested change

	current_system = "daint" # what system/cluster to execute commands on
	current_system = "daint" # Specifies the target cluster for command execution

#ai-review-inline

[github-actions]

Consider using a more descriptive comment explaining why this interval was chosen.

Suggested change

	update_check_interval_hours = 24
	update_check_interval_hours = 24 # Check for updates every 24 hours

#ai-review-inline

[github-actions]

Function signature should return Result<(), Report> for better error handling consistency.

Suggested change

	pub async fn check_update() -> () {
	pub async fn check_update() -> Result<()> {

#ai-review-inline

[github-actions]

Using unwrap() can cause panic; prefer proper error handling with ? operator.

Suggested change

	std::fs::write(&stamp_path, Local::now().to_rfc3339()).unwrap();
	std::fs::write(&stamp_path, Local::now().to_rfc3339()).map_err(|e| Report::msg(format!("Failed to update stamp file: {}", e)))?;

#ai-review-inline

[github-actions]

Ignoring the result of update() may hide potential update failures; consider logging or propagating the error.

Suggested change

	let _ = update();
	let result = update(); if let Err(e) = result { eprintln!("Update failed: {}", e); }

#ai-review-inline

[github-actions]

The time comparison may overflow if the interval is too large; consider using checked arithmetic.

Suggested change

	if now.naive_local() - update_stamp.naive_local() > TimeDelta::hours(config.values.update_check_interval as i64) {
	if (now.naive_local() - update_stamp.naive_local()).num_hours() > config.values.update_check_interval as i64 {

#ai-review-inline

[github-actions]

Using unwrap() can cause panic; prefer proper error handling with ? operator.

Suggested change

	std::fs::write(&stamp_path, Local::now().to_rfc3339()).unwrap();
	std::fs::write(&stamp_path, Local::now().to_rfc3339()).map_err(|e| Report::msg(format!("Failed to write stamp file: {}", e)))?;

#ai-review-inline