Analyze update (#30)

* remove --targets flag from analyze * handle_run tests and bug fixes * debugging github actions test * debugging github actions test * debug ci * debug ci * debug ci * debug ci * debug ci * debug ci * debug ci * debug ci * debug ci * debug ci * debug ci * debug ci * debug ci * debug ci * debug ci * debug ci
pnordahl · Nov 14, 2024 · 977e310 · 977e310
1 parent 50a34c5
commit 977e310
Show file tree

Hide file tree

Showing 13 changed files with 312 additions and 218 deletions.
diff --git a/.github/workflows/branch.yml b/.github/workflows/branch.yml
@@ -15,42 +15,16 @@ jobs:
   monorail:
     name: monorail
     runs-on: ubuntu-latest
+    env:
+      MONORAIL_TEST_DIR : ${{ github.workspace }}/tmp
     steps:
-      - name: prep
-        run: |
+      - uses: actions/checkout@v4
+      - uses: actions-rust-lang/setup-rust-toolchain@v1
+      - run: |
           sudo chown -R $USER:$USER ${{ github.workspace }}
+          mkdir -p "$MONORAIL_TEST_DIR"
           git config --global user.name "FIRST_NAME LAST_NAME"
           git config --global user.email "[email protected]"
-
-      - name: deps
-        uses: actions/checkout@v2
-
-      - name: rust stable toolchain
-        uses: actions-rs/toolchain@v1
-        with:
-          profile: minimal
-          toolchain: stable
-          override: true
-
-      - name: clippy
-        uses: actions-rs/cargo@v1
-        with:
-          toolchain: stable
-          command: clippy
-          args: -- -D warnings
-
-      - name: build
-        uses: actions-rs/cargo@v1
-        with:
-          toolchain: stable
-          command: build
-          args: --verbose --all
-
-      - name: test
-        uses: actions-rs/cargo@v1
-        with:
-          toolchain: stable
-          command: test
-          args: -- --nocapture
-
-
+      - run: cargo clippy -- -D warnings
+      - run: cargo build --verbose --all
+      - run: cargo test -- --nocapture
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,13 @@
 # Change Log
 
+## [3.5.1] - 2024-11-14
+
+### Changed
+Removed `--target (-t)` flag from `monorail analyze`
+
+### Fixed
+Issue preventing `monorail run` from returning non-zero exit codes
+
 ## [3.5.0] - 2024-11-13
 
 ### Changed

diff --git a/Cargo.toml b/Cargo.toml
@@ -4,7 +4,7 @@ description = "A tool for effective polyglot, multi-project monorepo development
 license = "MIT"
 homepage = "https://github.com/pnordahl/monorail"
 repository = "https://github.com/pnordahl/monorail"
-version = "3.5.0"
+version = "3.5.1"
 authors = ["Patrick Nordahl <[email protected]>"]
 edition = "2021"
 keywords = ["monorail", "monorepo", "build", "cli", "build-tool"]

diff --git a/README.md b/README.md
@@ -224,13 +224,14 @@ Defining a command can be done automatically by accepting `monorail` defaults, o
 
 #### Automatic
 
-By default, targets store their commands in a `monorail` directory relative to the target's path. The stem of the file becomes the command name. For example:
+By default, targets store their commands in a `monorail/cmd` directory relative to the target's path. The stem of the file becomes the command name. For example:
 
 ```sh
 repo/
   one/
     monorail/
-      build.sh
+      cmd/
+        build.sh
 ```
 
 This defines the `build` command for the `one` target.
@@ -299,7 +300,7 @@ This will run `build` first for `dep1`, and then `target1` and `target2` in para
 
 #### Arguments
 
-Commands can be provided with positional arguments at runtime. There are two mechanism for doing this; the base argmap, and as flags provided to `monorail run`. In your command executables, capture these positional arguments as you would in any program in that language.
+Commands can be provided with positional arguments at runtime. There are two mechanism for doing this; the base argmap, and as flags provided to `monorail run`. In your command executables, capture these positional arguments as you would in any program in that language. As is common, index 0 of the positional arguments is always an absolute path to the executable, so any additional arguments begin at index 1.
 
 ##### Base argmap
 The base argmap is an optional file containing argument mappings, which, if provided, is automatically loaded before any other argument mappings provided to `monorail run`. This is useful for specifying static parameterization for commands, especially when a command executable is generic and reused among multiple targets. An example of this useful pattern is shown in https://github.com/pnordahl/monorail-example in the rust crate targets.

diff --git a/src/api/cli.rs b/src/api/cli.rs
@@ -43,8 +43,8 @@ pub const ARG_STDOUT: &str = "stdout";
 pub const ARG_ID: &str = "id";
 pub const ARG_DEPS: &str = "deps";
 pub const ARG_ARG: &str = "arg";
-pub const ARG_ARG_MAP: &str = "argmap";
-pub const ARG_ARG_MAP_FILE: &str = "argmap-file";
+pub const ARG_ARGMAP: &str = "argmap";
+pub const ARG_ARGMAP_FILE: &str = "argmap-file";
 pub const ARG_FAIL_ON_UNDEFINED: &str = "fail-on-undefined";
 pub const ARG_NO_BASE_ARGMAPS: &str = "no-base-argmaps";
 
@@ -291,8 +291,8 @@ Refer to --help for more information on these options.
                 .long_help("This is a shorthand form of the more expressive '--target-argmap' and '--target-argmap-file', designed for single command + single target use. Providing this flag without specifying exactly one command and one target will result in an error.")
         )
         .arg(
-            Arg::new(ARG_ARG_MAP)
-                .long(ARG_ARG_MAP)
+            Arg::new(ARG_ARGMAP)
+                .long(ARG_ARGMAP)
                 .short('m')
                 .num_args(1..)
                 .required(false)
@@ -318,8 +318,8 @@ See `monorail run -h` for information on how this interacts with other arg-relat
 "#),
         )
         .arg(
-            Arg::new(ARG_ARG_MAP_FILE)
-                .long(ARG_ARG_MAP_FILE)
+            Arg::new(ARG_ARGMAP_FILE)
+                .long(ARG_ARGMAP_FILE)
                 .short('f')
                 .num_args(1..)
                 .required(false)
@@ -405,16 +405,7 @@ See `monorail run -h` for information on how this interacts with other arg-relat
                     .long(ARG_ALL)
                     .help("Display changes, change targets, and targets")
                     .action(ArgAction::SetTrue),
-            )
-            .arg(
-            Arg::new(ARG_TARGETS)
-                .short('t')
-                .long(ARG_TARGETS)
-                .required(false)
-                .num_args(1..)
-                .value_delimiter(' ')
-                .action(ArgAction::Append)
-                .help("Scope analysis to only the provided targets.")))
+            ))
     .subcommand(
         Command::new(CMD_OUT)
         .about("Manipulate data in the monorail output directory")
@@ -765,13 +756,13 @@ impl<'a> TryFrom<&'a clap::ArgMatches> for app::run::HandleRunInput<'a> {
                 .into_iter()
                 .flatten()
                 .collect(),
-            arg_map: cmd
-                .get_many::<String>(ARG_ARG_MAP)
+            argmap: cmd
+                .get_many::<String>(ARG_ARGMAP)
                 .into_iter()
                 .flatten()
                 .collect(),
-            arg_map_file: cmd
-                .get_many::<String>(ARG_ARG_MAP_FILE)
+            argmap_file: cmd
+                .get_many::<String>(ARG_ARGMAP_FILE)
                 .into_iter()
                 .flatten()
                 .collect(),
@@ -792,11 +783,6 @@ impl<'a> From<&'a clap::ArgMatches> for app::analyze::HandleAnalyzeInput<'a> {
                 end: cmd.get_one::<String>(ARG_END).map(|x: &String| x.as_str()),
                 git_path: cmd.get_one::<String>(ARG_GIT_PATH).unwrap(),
             },
-            targets: cmd
-                .get_many::<String>(ARG_TARGETS)
-                .into_iter()
-                .flatten()
-                .collect(),
             analyze_input: app::analyze::AnalyzeInput {
                 show_changes: cmd.get_flag(ARG_CHANGES),
                 show_change_targets: cmd.get_flag(ARG_CHANGE_TARGETS),

diff --git a/src/app/analyze.rs b/src/app/analyze.rs
@@ -13,7 +13,6 @@ use crate::core::{git, tracking};
 pub(crate) struct HandleAnalyzeInput<'a> {
     pub(crate) git_opts: git::GitOptions<'a>,
     pub(crate) analyze_input: AnalyzeInput,
-    pub(crate) targets: HashSet<&'a String>,
 }
 
 #[derive(Debug)]
@@ -87,30 +86,22 @@ pub(crate) async fn handle_analyze<'a>(
     input: &HandleAnalyzeInput<'a>,
     work_path: &'a path::Path,
 ) -> Result<AnalyzeOutput, MonorailError> {
-    let (mut index, changes) = match input.targets.len() {
-        0 => {
-            let changes = match cfg.change_provider.r#use {
-                ChangeProviderKind::Git => match cfg.change_provider.r#use {
-                    ChangeProviderKind::Git => {
-                        let tracking = tracking::Table::new(&cfg.get_tracking_path(work_path))?;
-                        let checkpoint = match tracking.open_checkpoint() {
-                            Ok(checkpoint) => Some(checkpoint),
-                            Err(MonorailError::TrackingCheckpointNotFound(_)) => None,
-                            Err(e) => {
-                                return Err(e);
-                            }
-                        };
-                        git::get_git_all_changes(&input.git_opts, &checkpoint, work_path).await?
+    let changes = match cfg.change_provider.r#use {
+        ChangeProviderKind::Git => match cfg.change_provider.r#use {
+            ChangeProviderKind::Git => {
+                let tracking = tracking::Table::new(&cfg.get_tracking_path(work_path))?;
+                let checkpoint = match tracking.open_checkpoint() {
+                    Ok(checkpoint) => Some(checkpoint),
+                    Err(MonorailError::TrackingCheckpointNotFound(_)) => None,
+                    Err(e) => {
+                        return Err(e);
                     }
-                },
-            };
-            (
-                core::Index::new(cfg, &cfg.get_target_path_set(), work_path)?,
-                changes,
-            )
-        }
-        _ => (core::Index::new(cfg, &input.targets, work_path)?, None),
+                };
+                git::get_git_all_changes(&input.git_opts, &checkpoint, work_path).await?
+            }
+        },
     };
+    let mut index = core::Index::new(cfg, &cfg.get_target_path_set(), work_path)?;
 
     analyze(&input.analyze_input, &mut index, changes)
 }
@@ -393,12 +384,11 @@ pub(crate) fn analyze(
 mod tests {
     use super::*;
     use crate::core::testing::*;
-    use tempfile::tempdir;
 
     #[tokio::test]
     async fn test_analyze_empty() {
         let changes = vec![];
-        let td = tempdir().unwrap();
+        let td = new_testdir().unwrap();
         let work_path = &td.path();
         let c = new_test_repo(work_path).await;
         let mut index = core::Index::new(&c, &c.get_target_path_set(), work_path).unwrap();
@@ -421,7 +411,7 @@ mod tests {
             targets: Some(vec![]),
         }];
 
-        let td = tempdir().unwrap();
+        let td = new_testdir().unwrap();
         let work_path = &td.path();
         let c = new_test_repo(work_path).await;
         let mut index = core::Index::new(&c, &c.get_target_path_set(), work_path).unwrap();
@@ -450,7 +440,7 @@ mod tests {
             }]),
         }];
 
-        let td = tempdir().unwrap();
+        let td = new_testdir().unwrap();
         let work_path = &td.path();
         let c = new_test_repo(work_path).await;
         let mut index = core::Index::new(&c, &c.get_target_path_set(), work_path).unwrap();
@@ -478,7 +468,7 @@ mod tests {
             }]),
         }];
 
-        let td = tempdir().unwrap();
+        let td = new_testdir().unwrap();
         let work_path = &td.path();
         let c = new_test_repo(work_path).await;
         let mut index = core::Index::new(&c, &c.get_target_path_set(), work_path).unwrap();
@@ -518,7 +508,7 @@ mod tests {
             ]),
         }];
 
-        let td = tempdir().unwrap();
+        let td = new_testdir().unwrap();
         let work_path = &td.path();
         let c = new_test_repo(work_path).await;
         let mut index = core::Index::new(&c, &c.get_target_path_set(), work_path).unwrap();
@@ -554,7 +544,7 @@ mod tests {
             ]),
         }];
 
-        let td = tempdir().unwrap();
+        let td = new_testdir().unwrap();
         let work_path = &td.path();
         let c = new_test_repo(work_path).await;
         let mut index = core::Index::new(&c, &c.get_target_path_set(), work_path).unwrap();
@@ -604,7 +594,7 @@ mod tests {
             },
         ];
 
-        let td = tempdir().unwrap();
+        let td = new_testdir().unwrap();
         let work_path = &td.path();
         let c = new_test_repo(work_path).await;
         let mut index = core::Index::new(&c, &c.get_target_path_set(), work_path).unwrap();

diff --git a/src/app/checkpoint.rs b/src/app/checkpoint.rs
@@ -107,11 +107,10 @@ mod tests {
     use super::*;
     use crate::core::git;
     use crate::core::testing::*;
-    use tempfile::tempdir;
 
     #[tokio::test]
     async fn test_handle_checkpoint_delete_success() {
-        let td2 = tempdir().unwrap();
+        let td2 = new_testdir().unwrap();
         let rp = &td2.path();
         let cfg = new_test_repo(rp).await;
         let tracking_path = cfg.get_tracking_path(rp);
@@ -131,7 +130,7 @@ mod tests {
 
     #[tokio::test]
     async fn test_handle_checkpoint_show_success() {
-        let td2 = tempdir().unwrap();
+        let td2 = new_testdir().unwrap();
         let rp = &td2.path();
         let cfg = new_test_repo(rp).await;
         let tracking_path = cfg.get_tracking_path(rp);
@@ -151,7 +150,7 @@ mod tests {
 
     #[tokio::test]
     async fn test_handle_checkpoint_update_with_pending_changes() {
-        let td2 = tempdir().unwrap();
+        let td2 = new_testdir().unwrap();
         let rp = &td2.path();
         let cfg = new_test_repo(rp).await;
         let head = get_head(rp).await;
@@ -166,17 +165,13 @@ mod tests {
         let result = handle_checkpoint_update(&cfg, &input, rp).await;
         assert!(result.is_ok());
         let output = result.unwrap();
-        assert!(
-            // number of files in the test config
-            output.checkpoint.pending.unwrap().len() == 17,
-            "Pending changes should be populated"
-        );
+        assert!(!output.checkpoint.pending.unwrap().is_empty());
         assert_eq!(output.checkpoint.id, head);
     }
 
     #[tokio::test]
     async fn test_handle_checkpoint_update_no_pending_changes() {
-        let td2 = tempdir().unwrap();
+        let td2 = new_testdir().unwrap();
         let rp = &td2.path();
         let cfg = new_test_repo(rp).await;
         let git_opts: git::GitOptions = Default::default();

diff --git a/src/app/log.rs b/src/app/log.rs
@@ -499,11 +499,11 @@ pub(crate) fn get_header(filename: &str, target: &str, command: &str, color: boo
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::core::testing::*;
     use std::sync::{
         atomic::{AtomicBool, Ordering},
         Arc,
     };
-    use tempfile::tempdir;
 
     #[test]
     fn test_compressor_initialization() {
@@ -521,7 +521,7 @@ mod tests {
     fn test_register_paths() {
         let shutdown = Arc::new(AtomicBool::new(false));
         let mut compressor = Compressor::new(2, shutdown.clone());
-        let temp_dir = tempdir().unwrap();
+        let temp_dir = new_testdir().unwrap();
 
         // Register a path and validate the returned CompressorClient
         let test_path = temp_dir.path().join("test_file_1");
@@ -549,7 +549,7 @@ mod tests {
     fn test_run_compressor_with_data_requests() {
         let shutdown = Arc::new(AtomicBool::new(false));
         let mut compressor = Compressor::new(1, shutdown.clone());
-        let temp_dir = tempdir().unwrap();
+        let temp_dir = new_testdir().unwrap();
 
         let test_path = temp_dir.path().join("test_output.zst");
         let client = compressor
@@ -584,7 +584,7 @@ mod tests {
     fn test_shutdown_compressor() {
         let shutdown = Arc::new(AtomicBool::new(false));
         let mut compressor = Compressor::new(2, shutdown.clone());
-        let temp_dir = tempdir().unwrap();
+        let temp_dir = new_testdir().unwrap();
 
         // Register paths
         let test_path_1 = temp_dir.path().join("test_file_1");