fix: Audit 08/15 (#1614)

* fix: Fix required approvals not working with 1.

* fix: Track passthrough for CI skipped tasks.

* tests: Add dependency chain tests.

.yarn/versions/92145757.yml

@@ -0,0 +1,9 @@
+releases:
+  "@moonrepo/cli": patch
+  "@moonrepo/core-linux-arm64-gnu": patch
+  "@moonrepo/core-linux-arm64-musl": patch
+  "@moonrepo/core-linux-x64-gnu": patch
+  "@moonrepo/core-linux-x64-musl": patch
+  "@moonrepo/core-macos-arm64": patch
+  "@moonrepo/core-macos-x64": patch
+  "@moonrepo/core-windows-x64-msvc": patch

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Changelog
 
+## Unreleased
+
+#### 🐞 Fixes
+
+- Fixed an issue where codeowners `requiredAppprovals` wouldn't allow `1`.
+- Fixed an issue where a task that depends on another task that has `runInCI: false`, would not run
+  as affected in `moon ci` because the dependency task was skipped.
+
 ## 1.27.6
 
 #### 📢 Notice

crates/action-context/src/lib.rs

@@ -1,8 +1,7 @@
 use clap::ValueEnum;
 use moon_common::path::WorkspaceRelativePathBuf;
 use moon_target::{Target, TargetLocator};
-use rustc_hash::FxHashSet;
-use scc::HashMap;
+use rustc_hash::{FxHashMap, FxHashSet};
 use serde::{Deserialize, Serialize};
 use std::sync::Arc;
 use tokio::sync::Mutex;
@@ -48,7 +47,7 @@ pub struct ActionContext {
     /// Active mutexes for tasks to acquire locks against.
     /// @mutable
     #[serde(skip)]
-    pub named_mutexes: HashMap<String, Arc<Mutex<()>>>,
+    pub named_mutexes: scc::HashMap<String, Arc<Mutex<()>>>,
 
     /// Additional arguments passed after `--` to passthrough.
     pub passthrough_args: Vec<String>,
@@ -61,7 +60,7 @@ pub struct ActionContext {
 
     /// The current state of running tasks (via their target).
     /// @mutable
-    pub target_states: HashMap<Target, TargetState>,
+    pub target_states: scc::HashMap<Target, TargetState>,
 
     /// Files that have currently been touched.
     pub touched_files: FxHashSet<WorkspaceRelativePathBuf>,
@@ -82,6 +81,14 @@ impl ActionContext {
         mutex
     }
 
+    pub fn get_target_states(&self) -> FxHashMap<Target, TargetState> {
+        let mut map = FxHashMap::default();
+        self.target_states.scan(|k, v| {
+            map.insert(k.to_owned(), v.to_owned());
+        });
+        map
+    }
+
     pub fn is_primary_target<T: AsRef<Target>>(&self, target: T) -> bool {
         self.primary_targets.contains(target.as_ref())
     }

crates/action-graph/Cargo.toml

@@ -10,6 +10,7 @@ publish = false
 
 [dependencies]
 moon_action = { path = "../action" }
+moon_action_context = { path = "../action-context" }
 moon_common = { path = "../common" }
 moon_config = { path = "../config" }
 # TODO remove

crates/action-graph/src/action_graph_builder.rs

@@ -3,6 +3,7 @@ use moon_action::{
     ActionNode, InstallProjectDepsNode, InstallWorkspaceDepsNode, RunTaskNode, SetupToolchainNode,
     SyncProjectNode,
 };
+use moon_action_context::{ActionContext, TargetState};
 use moon_common::Id;
 use moon_common::{color, path::WorkspaceRelativePathBuf};
 use moon_config::{PlatformType, TaskDependencyConfig};
@@ -13,6 +14,7 @@ use moon_query::{build_query, Criteria};
 use moon_task::{parse_task_args, Target, TargetError, TargetLocator, TargetScope, Task};
 use petgraph::prelude::*;
 use rustc_hash::{FxHashMap, FxHashSet};
+use std::mem;
 use tracing::{debug, instrument, trace};
 
 type TouchedFilePaths = FxHashSet<WorkspaceRelativePathBuf>;
@@ -41,6 +43,10 @@ pub struct ActionGraphBuilder<'app> {
     indices: FxHashMap<ActionNode, NodeIndex>,
     platform_manager: &'app PlatformManager,
     project_graph: &'app ProjectGraph,
+
+    // Target tracking
+    passthrough_targets: FxHashSet<Target>,
+    primary_targets: FxHashSet<Target>,
 }
 
 impl<'app> ActionGraphBuilder<'app> {
@@ -58,13 +64,31 @@ impl<'app> ActionGraphBuilder<'app> {
             all_query: None,
             graph: DiGraph::new(),
             indices: FxHashMap::default(),
+            passthrough_targets: FxHashSet::default(),
             platform_manager,
+            primary_targets: FxHashSet::default(),
             project_graph,
         })
     }
 
-    pub fn build(self) -> miette::Result<ActionGraph> {
-        Ok(ActionGraph::new(self.graph))
+    pub fn build(self) -> ActionGraph {
+        ActionGraph::new(self.graph)
+    }
+
+    pub fn build_context(&mut self) -> ActionContext {
+        let mut context = ActionContext::default();
+
+        if !self.passthrough_targets.is_empty() {
+            for target in mem::take(&mut self.passthrough_targets) {
+                context.set_target_state(target, TargetState::Passthrough);
+            }
+        }
+
+        if !self.primary_targets.is_empty() {
+            context.primary_targets = mem::take(&mut self.primary_targets);
+        }
+
+        context
     }
 
     pub fn get_index_from_node(&self, node: &ActionNode) -> Option<&NodeIndex> {
@@ -188,13 +212,31 @@ impl<'app> ActionGraphBuilder<'app> {
         // but not `moon run`, since the latter should be able to
         // manually run local tasks in CI (deploys, etc).
         if reqs.ci && reqs.ci_check && !task.should_run_in_ci() {
+            self.passthrough_targets.insert(task.target.clone());
+
             debug!(
                 task = task.target.as_str(),
                 "Not running task {} because {} is false",
                 color::label(&task.target.id),
                 color::property("runInCI"),
             );
 
+            // Dependents may still want to run though!
+            if reqs.dependents {
+                self.run_task_dependents(task, reqs)?;
+            }
+
+            return Ok(None);
+        }
+
+        // These tasks shouldn't actually run, so filter them out
+        if self.passthrough_targets.contains(&task.target) {
+            trace!(
+                task = task.target.as_str(),
+                "Not adding task {} to graph because it has been marked as passthrough",
+                color::label(&task.target.id),
+            );
+
             return Ok(None);
         }
 
@@ -270,10 +312,7 @@ impl<'app> ActionGraphBuilder<'app> {
         let mut previous_target_index = None;
 
         for dep in &task.deps {
-            let (_, dep_indices) =
-                self.run_task_by_target_with_config(&dep.target, &reqs, Some(dep))?;
-
-            for dep_index in dep_indices {
+            for dep_index in self.run_task_by_target_with_config(&dep.target, &reqs, Some(dep))? {
                 // When parallel, parent depends on child
                 if parallel {
                     indices.push(dep_index);
@@ -321,10 +360,10 @@ impl<'app> ActionGraphBuilder<'app> {
             }
 
             for project in projects_to_build {
-                // Include internal tasks!
+                // Don't skip internal tasks, since they are a dependency of the parent
+                // task, and must still run! They just can't be ran manually.
                 for dep_task in project.tasks.values() {
-                    // Don't skip internal tasks, since they are a dependency of the parent
-                    // task, and must still run! They just can't be ran manually.
+                    // But do skip persistent tasks!
                     if dep_task.is_persistent() {
                         continue;
                     }
@@ -333,6 +372,7 @@ impl<'app> ActionGraphBuilder<'app> {
                     // that should not run in CI, as we don't know what side-effects it
                     // will cause. This applies to both `moon ci` and `moon run`.
                     if parent_reqs.ci && !dep_task.should_run_in_ci() {
+                        self.passthrough_targets.insert(dep_task.target.clone());
                         continue;
                     }
 
@@ -352,15 +392,15 @@ impl<'app> ActionGraphBuilder<'app> {
         &mut self,
         target: T,
         reqs: &RunRequirements<'app>,
-    ) -> miette::Result<(FxHashSet<Target>, FxHashSet<NodeIndex>)> {
+    ) -> miette::Result<FxHashSet<NodeIndex>> {
         self.run_task_by_target_with_config(target, reqs, None)
     }
 
     pub fn run_task_by_target_locator<T: AsRef<TargetLocator>>(
         &mut self,
         target_locator: T,
         reqs: &mut RunRequirements<'app>,
-    ) -> miette::Result<(FxHashSet<Target>, FxHashSet<NodeIndex>)> {
+    ) -> miette::Result<FxHashSet<NodeIndex>> {
         match target_locator.as_ref() {
             TargetLocator::Qualified(target) => self.run_task_by_target(target, reqs),
             TargetLocator::TaskFromWorkingDir(task_id) => {
@@ -380,7 +420,7 @@ impl<'app> ActionGraphBuilder<'app> {
         target: T,
         reqs: &RunRequirements<'app>,
         config: Option<&TaskDependencyConfig>,
-    ) -> miette::Result<(FxHashSet<Target>, FxHashSet<NodeIndex>)> {
+    ) -> miette::Result<FxHashSet<NodeIndex>> {
         let target = target.as_ref();
         let mut inserted_targets = FxHashSet::default();
         let mut inserted_indices = FxHashSet::default();
@@ -463,7 +503,9 @@ impl<'app> ActionGraphBuilder<'app> {
             }
         };
 
-        Ok((inserted_targets, inserted_indices))
+        self.primary_targets.extend(inserted_targets);
+
+        Ok(inserted_indices)
     }
 
     #[instrument(skip_all)]

crates/action-graph/tests/__fixtures__/tasks/ci/moon.yml

@@ -0,0 +1,36 @@
+tasks:
+  ci1-dependency:
+    options:
+      runInCI: false
+  ci1-dependant:
+    deps:
+      - ci1-dependency
+    options:
+      runInCI: true
+
+  ci2-dependency:
+    options:
+      runInCI: false
+  ci2-dependant:
+    deps:
+      - ci2-dependency
+    options:
+      runInCI: false
+
+  ci3-dependency:
+    options:
+      runInCI: true
+  ci3-dependant:
+    deps:
+      - ci2-dependency
+    options:
+      runInCI: false
+
+  ci4-dependency:
+    options:
+      runInCI: true
+  ci4-dependant:
+    deps:
+      - ci4-dependency
+    options:
+      runInCI: true