-
Notifications
You must be signed in to change notification settings - Fork 5
Fix for the workflow cli create cmd #1294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e3acf7e
f8f776f
5b8952e
820e4ed
4baa2ee
07090f0
b50fc55
7732954
493c3f7
276c47f
77ff9db
20b1fc5
a569491
be09a72
0abb419
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this necessary for this PR? I don't think that it shouldn't be included in the
|
||
| apiVersion: argoproj.io/v1alpha1 | ||
| kind: Workflow | ||
| metadata: | ||
| name: hello-world-cli-test | ||
| labels: | ||
| workflows.diamond.ac.uk/science-group-examples: "true" | ||
| annotations: | ||
| workflows.argoproj.io/title: Example | ||
| workflows.argoproj.io/description: | | ||
| This workflow is used to test the workflows CLI | ||
| with a simple long-running Hello World example. | ||
| workflows.diamond.ac.uk/repository: "https://github.com/DiamondLightSource/workflows" | ||
| spec: | ||
| entrypoint: hello-sleep | ||
| templates: | ||
| - name: hello-sleep | ||
| container: | ||
| image: alpine:3.18 | ||
| command: [sh, -c] | ||
| args: | ||
| - | | ||
| echo "Hello World from workflows CLI" | ||
| echo "Sleeping for 20 seconds... Zz... ZZzzz.." | ||
| sleep 20 | ||
| echo "Done" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| use crate::CreateArgs; | ||
| use std::io; | ||
| use std::os::unix::fs as fs_sym; | ||
| use std::path::{Path, PathBuf}; | ||
| use std::path::Path; | ||
| use std::{fs, process}; | ||
|
|
||
| pub fn create(args: CreateArgs) { | ||
|
|
@@ -19,38 +19,66 @@ pub fn create(args: CreateArgs) { | |
| } | ||
|
|
||
| fn generate_template_repo(args: &CreateArgs, prompt_fn: fn(&str) -> bool) -> Result<(), String> { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a lot of change here, some of it seems to be refactoring. What actually was the cause of the bug? And what was the fix? |
||
| // Reject workflow files | ||
| if args.name.ends_with(".yaml") || args.name.ends_with(".yml") { | ||
| return Err(format!( | ||
| "Invalid name '{}': create expects a directory name, not a workflow file", | ||
| args.name | ||
| )); | ||
| } | ||
|
|
||
| let root_path = Path::new(&args.name); | ||
| println!("Generating Template Repo: {:?}", args.name); | ||
| let new_dir = fs::create_dir(root_path); | ||
| match new_dir { | ||
| Ok(_) => println!("Created directory: {}", args.name), | ||
| Err(e) => { | ||
| let err = format!("Failed to create directory {}: {}", args.name, e); | ||
| return Err(err); | ||
| } | ||
| println!("Generating Template Repo: {}", root_path.display()); | ||
|
|
||
| fs::create_dir(root_path) | ||
| .map_err(|e| format!("Failed to create directory {}: {}", root_path.display(), e))?; | ||
|
|
||
| println!("Created directory: {}", root_path.display()); | ||
|
|
||
| let workflows_home = Path::new(&args.workflows_home); | ||
|
|
||
| let conventional_src = workflows_home.join("template-boilerplate/conventional-templates"); | ||
| let helm_src = workflows_home.join("template-boilerplate/helm-based-templates"); | ||
|
|
||
| // Validate sources BEFORE asking user anything | ||
| if !conventional_src.exists() { | ||
| return Err(format!( | ||
| "Missing conventional template source directory: {}", | ||
| conventional_src.display() | ||
| )); | ||
| } | ||
| if !helm_src.exists() { | ||
| return Err(format!( | ||
| "Missing helm template source directory: {}", | ||
| helm_src.display() | ||
| )); | ||
| } | ||
|
|
||
| let conventional_dest = root_path.join("conventional-templates"); | ||
| let helm_dest = root_path.join("helm-based-templates"); | ||
| let workflow_home_path = Path::new(&args.workflows_home); | ||
| let conventional_src = workflow_home_path.join("template-boilerplate/conventional-templates"); | ||
| let helm_src = workflow_home_path.join("template-boilerplate/helm-based-templates"); | ||
|
|
||
| if !args.manifest { | ||
| if prompt_fn("Would you like to store conventional WorkflowTemplate manifests? (y/n)") { | ||
| copy_directory(&conventional_src, &conventional_dest); | ||
| } | ||
| let include_conventional = if args.manifest { | ||
| true | ||
| } else { | ||
| copy_directory(&conventional_src, &conventional_dest); | ||
| prompt_fn("Would you like to store conventional WorkflowTemplate manifests? (y/n)") | ||
| }; | ||
|
|
||
| if include_conventional { | ||
| copy_directory(&conventional_src, &conventional_dest)?; | ||
| println!("Copied conventional templates"); | ||
| } | ||
|
|
||
| if !args.helm { | ||
| if prompt_fn("Would you like to store helm-based WorkflowTemplates? (y/n)") { | ||
| copy_directory(&helm_src, &helm_dest); | ||
| } | ||
| let include_helm = if args.helm { | ||
| true | ||
| } else { | ||
| copy_directory(&helm_src, &helm_dest); | ||
| prompt_fn("Would you like to store helm-based WorkflowTemplates? (y/n)") | ||
| }; | ||
|
|
||
| if include_helm { | ||
| copy_directory(&helm_src, &helm_dest)?; | ||
| println!("Copied helm templates"); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
|
|
@@ -59,61 +87,69 @@ fn prompt(message: &str) -> bool { | |
| let mut selection = String::new(); | ||
|
|
||
| loop { | ||
| selection.clear(); | ||
|
|
||
| io::stdin() | ||
| .read_line(&mut selection) | ||
| .expect("Failed to read line"); | ||
| if selection.to_lowercase().trim() == "y" { | ||
| return true; | ||
| } else if selection.to_lowercase().trim() == "n" { | ||
| return false; | ||
| } else { | ||
| println!("Invalid input. Please enter 'y' or 'n'."); | ||
|
|
||
| match selection.trim().to_lowercase().as_str() { | ||
| "y" => return true, | ||
| "n" => return false, | ||
| _ => println!("Invalid input. Please enter 'y' or 'n'."), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn copy_directory(src: &PathBuf, dest: &PathBuf) { | ||
| if let Err(e) = fs::create_dir_all(dest) { | ||
| eprintln!( | ||
| /// Recursively copy directory contents | ||
| fn copy_directory(src: &Path, dest: &Path) -> Result<(), String> { | ||
| fs::create_dir_all(dest).map_err(|e| { | ||
| format!( | ||
| "Failed to create destination directory {}: {}", | ||
| dest.display(), | ||
| e | ||
| ); | ||
| return; | ||
| } | ||
| if let Ok(entries) = fs::read_dir(src) { | ||
| for entry in entries.flatten() { | ||
| let src_path = entry.path(); | ||
| let dest_path = dest.join(entry.file_name()); | ||
|
|
||
| if src_path.is_symlink() { | ||
| if let Ok(link_target) = fs::read_link(&src_path) { | ||
| #[cfg(unix)] | ||
| if let Err(e) = fs_sym::symlink(&link_target, &dest_path) { | ||
| eprintln!("Failed to create symlink {}: {}", dest_path.display(), e); | ||
| } | ||
| } | ||
| } else if src_path.is_dir() { | ||
| if let Err(e) = fs::create_dir_all(&dest_path) { | ||
| eprintln!("Failed to create directory {}: {}", dest_path.display(), e); | ||
| continue; | ||
| } | ||
| copy_directory(&src_path, &dest_path); | ||
| } else if src_path.is_file() | ||
| && let Err(e) = fs::copy(&src_path, &dest_path) | ||
| { | ||
| eprintln!("Failed to copy {}: {}", src_path.display(), e); | ||
| } | ||
| ) | ||
| })?; | ||
|
|
||
| for entry in fs::read_dir(src) | ||
| .map_err(|e| format!("Failed to read source folder {}: {}", src.display(), e))? | ||
| { | ||
| let entry = entry.map_err(|e| e.to_string())?; | ||
|
|
||
| let src_path = entry.path(); | ||
| let dest_path = dest.join(entry.file_name()); | ||
|
|
||
| let metadata = fs::symlink_metadata(&src_path) | ||
| .map_err(|e| format!("Failed to read metadata for {}: {}", src_path.display(), e))?; | ||
|
|
||
| // SYMLINK | ||
| if metadata.file_type().is_symlink() { | ||
| let target = fs::read_link(&src_path) | ||
| .map_err(|e| format!("Failed to read symlink {}: {}", src_path.display(), e))?; | ||
|
|
||
| fs_sym::symlink(&target, &dest_path) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you seem to have lost #[cfg(unix)]? |
||
| .map_err(|e| format!("Failed to create symlink {}: {}", dest_path.display(), e))?; | ||
| } else if metadata.is_dir() { | ||
| copy_directory(&src_path, &dest_path)?; | ||
| } else if metadata.is_file() { | ||
| fs::copy(&src_path, &dest_path).map_err(|e| { | ||
| format!( | ||
| "Failed to copy file {} -> {}: {}", | ||
| src_path.display(), | ||
| dest_path.display(), | ||
| e | ||
| ) | ||
| })?; | ||
| } | ||
| } else { | ||
| eprintln!("Failed to read source folder: {}", src.display()); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use ::serial_test::serial; | ||
| use serial_test::serial; | ||
| struct TestCleanup { | ||
| directory: String, | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this edit seems un-related to this pull request?