only use coman init system for docker images

[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 Conditional Init System Usage The logic for determining when to use coman as an init system has been updated to only activate when a Docker image is specified. This change should be validated to ensure it correctly prevents unintended usage of the init system in non-container environments. if let Some(path) = coman_squash { context.insert("coman_squash", &path); // path to coman squash file on remote // whether to use coman as an init system. Only do this if running an image context.insert("coman_init", &options.image.is_some()); } Warning Message Clarity A warning message has been added to inform users when no Docker image is specified. It should be verified that this message is clear and helpful for users who might expect SSH or port forwarding functionality without a container. println!( "Warning: No docker image specified (-i), functionality like SSH and port forwarding only works when running a docker image" ); None }; Template Parameter Update The sbatch_script_template now includes new template parameters (coman_squash and coman_init) and updates the conditional execution logic. Ensure that these changes align with the intended behavior and that the template correctly handles cases where these parameters are not set. # coman_squash: the path to the coman squash file # coman_init: whether to use coman as an init system for the command. Needed for SSH/Portforwarding. Only work when using a docker container sbatch_script_template = """ #!/bin/bash #SBATCH --job-name={{name}} #SBATCH --ntasks=1 #SBATCH --time=10:00:00 srun {% if environment_file %}--environment={{environment_file}}{% endif %} {% if coman_init %}/coman/coman exec {% endif %}{{command}} """

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Correctly conditionally invoke coman exec in sbatch template The template conditionally includes /coman/coman exec based on coman_init, which correctly reflects that coman should only be invoked when running within a Docker container. This ensures proper execution flow for containerized jobs. coman/.config/config.toml [24-32] + # coman_squash: the path to the coman squash file + # coman_init: whether to use coman as an init system for the command. Needed for SSH/Portforwarding. Only work when using a docker container + sbatch_script_template = """ + #!/bin/bash + #SBATCH --job-name={{name}} + #SBATCH --ntasks=1 + #SBATCH --time=10:00:00 +-srun {% if environment_file %}--environment={{environment_file}}{% endif %} {% if coman_squash %}/coman/coman exec {% endif %}{{command}} ++srun {% if environment_file %}--environment={{environment_file}}{% endif %} {% if coman_init %}/coman/coman exec {% endif %}{{command}} + """ - Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a key change in the template logic from using coman_squash to coman_init for conditional execution of /coman/coman exec. This is important for ensuring that coman is only invoked when appropriate, enhancing correctness and preventing unintended behavior in non-containerized environments.	Medium
	Replace println with structured logging Consider using a more structured logging approach instead of println! for warnings, especially in a library or application context where log levels and outputs may need to be configurable. coman/src/cscs/handlers.rs [850-852] - println!( - "Warning: No docker image specified (-i), functionality like SSH and port forwarding only works when running a docker image" - ); + tracing::warn!("No docker image specified (-i), functionality like SSH and port forwarding only works when running a docker image"); Suggestion importance[1-10]: 6 __ Why: The suggestion proposes using tracing::warn! instead of println! for better logging practices. While this is a good improvement for maintainability and configurability, it's not critical for correctness and doesn't address any immediate bugs or security issues.	Low
	Ensure coman init system is properly conditional on image presence The logic for setting coman_init should be inverted to enable coman as init system only when an image is present. Currently, it's set to true when an image exists, which is correct, but the comment suggests it should only be enabled for images, so ensure this behavior aligns with intended usage. coman/src/cscs/handlers.rs [775-777] + context.insert("coman_squash", &path); // path to coman squash file on remote + // whether to use coman as an init system. Only do this if running an image + context.insert("coman_init", &options.image.is_some()); - Suggestion importance[1-10]: 5 __ Why: The suggestion points out a potential logic issue with coman_init flag, but the existing code already correctly sets it based on options.image.is_some(). The improved code provided is identical to the existing code, indicating no actual change is needed, hence a low score.	Low