fix: Fixed various bugs:

1. Apply order fix (re-order by eval order since parallel plans mess up the order)
2. Added additional tests to validate the new logic.
3. Type fixes
4. Refactoring
5. Plan integration tests

Author: kevinwang5658

README.md

@@ -45,7 +45,7 @@ USAGE
 FLAGS
   -o, --output=<option>  [default: default]
                          <options: plain|default|debug|json>
-  -p, --path=<value>     path to project
+  -p, --path=<value>     Path to codify.json file
   -s, --secure
   --debug
 
@@ -70,11 +70,12 @@ Destroy or uninstall a resource (or many resources).
 
 ```
 USAGE
-  $ codify destroy [--json] [--debug] [-o plain|default|debug|json] [-s]
+  $ codify destroy [--json] [--debug] [-o plain|default|debug|json] [-s] [-p <value>]
 
 FLAGS
   -o, --output=<option>  [default: default]
                          <options: plain|default|debug|json>
+  -p, --path=<value>     Path to codify.json file
   -s, --secure
   --debug
 
@@ -121,7 +122,7 @@ USAGE
 FLAGS
   -o, --output=<option>  [default: default]
                          <options: plain|default|debug|json>
-  -p, --path=<value>     path to project
+  -p, --path=<value>     Path to codify.json file
   -s, --secure
   --debug
 
@@ -148,7 +149,7 @@ USAGE
 FLAGS
   -o, --output=<option>  [default: default]
                          <options: plain|default|debug|json>
-  -p, --path=<value>     path to project
+  -p, --path=<value>     Path to codify.json file
   -s, --secure
   --debug
 

src/entities/plan.test.ts

@@ -0,0 +1,78 @@
+import { expect, describe, it } from 'vitest';
+import { ResourceOperation } from 'codify-schemas';
+
+import { Plan, ResourcePlan } from './plan.js';
+import { Project } from './project.js';
+import { ResourceConfig } from './resource-config.js';
+
+describe('Unit tests for Plan entity', () => {
+  it('Can sort itself by eval order (useful since plans occur in parallel now and can be return in random order', () => {
+    const project = new Project(
+      null,
+      [
+        new ResourceConfig({ type: 'type1' }),
+        new ResourceConfig({ type: 'type2', dependsOn: ['type1'] }),
+        new ResourceConfig({ type: 'type3', dependsOn: ['type1', 'type2']}),
+      ],
+      './somewhere',
+    );
+
+    project.resolveDependenciesAndCalculateEvalOrder()
+    console.log(project.evaluationOrder)
+
+    expect(project.evaluationOrder!.length).to.eq(3);
+    expect(project.evaluationOrder![0]).to.eq('type1');
+    expect(project.evaluationOrder![1]).to.eq('type2');
+    expect(project.evaluationOrder![2]).to.eq('type3');
+
+    // Here we simulate receiving a plan that is out of order.
+    const plan = new Plan([
+      new ResourcePlan({
+        planId: '1',
+        operation: ResourceOperation.CREATE,
+        resourceName: undefined,
+        resourceType: 'type3',
+        isStateful: false,
+        parameters: []
+      }),
+      new ResourcePlan({
+        planId: '1',
+        operation: ResourceOperation.CREATE,
+        resourceName: undefined,
+        resourceType: 'type1',
+        isStateful: false,
+        parameters: []
+      }),
+      new ResourcePlan({
+        planId: '1',
+        operation: ResourceOperation.CREATE,
+        resourceName: undefined,
+        resourceType: 'type2',
+        isStateful: false,
+        parameters: []
+      })
+    ], project);
+
+    expect(plan.raw.length).to.eq(3);
+    expect(plan.raw[0].resourceType).to.eq('type3');
+    expect(plan.raw[1].resourceType).to.eq('type1');
+    expect(plan.raw[2].resourceType).to.eq('type2');
+
+    expect(plan.resources.length).to.eq(3);
+    expect(plan.resources[0].resourceType).to.eq('type3');
+    expect(plan.resources[1].resourceType).to.eq('type1');
+    expect(plan.resources[2].resourceType).to.eq('type2');
+
+    plan.sortByEvalOrder(project.evaluationOrder);
+    expect(plan.raw.length).to.eq(3);
+    expect(plan.raw[0].resourceType).to.eq('type1');
+    expect(plan.raw[1].resourceType).to.eq('type2');
+    expect(plan.raw[2].resourceType).to.eq('type3');
+
+    expect(plan.resources.length).to.eq(3);
+    expect(plan.resources[0].resourceType).to.eq('type1');
+    expect(plan.resources[1].resourceType).to.eq('type2');
+    expect(plan.resources[2].resourceType).to.eq('type3');
+
+  })
+})

src/entities/plan.ts

@@ -1,5 +1,6 @@
 import { ParameterOperation, PlanResponseData, ResourceConfig, ResourceOperation } from 'codify-schemas';
 
+import { getId } from '../utils/index.js';
 import { Project } from './project.js';
 
 export class Plan {
@@ -12,6 +13,33 @@ export class Plan {
     this.resources = resourcePlans
     this.project = project
   }
+
+  // This sorting is necessary with parallel plans because the order may be jumbled in the process
+  sortByEvalOrder(evalOrder: null | string[]) {
+    if (!evalOrder) {
+      return;
+    }
+
+    this.raw.sort((a, b) => {
+      const aId = getId(a.resourceType, a.resourceName);
+      const bId = getId(b.resourceType, b.resourceName);
+
+      const indexA = evalOrder.indexOf(aId);
+      const indexB = evalOrder.indexOf(bId);
+
+      return indexA > indexB ? 1 : -1;
+    });
+
+    this.resources.sort((a, b) => {
+      const aId = getId(a.resourceType, a.resourceName);
+      const bId = getId(b.resourceType, b.resourceName);
+
+      const indexA = evalOrder.indexOf(aId);
+      const indexB = evalOrder.indexOf(bId);
+
+      return indexA > indexB ? 1 : -1;
+    })
+  }
 
   getResourcePlan(id: string): ResourcePlan | null {
     return this.resources.find((r) => r.id === id) ?? null;

src/entities/project.ts

@@ -1,4 +1,4 @@
-import { ValidateResponseData, PlanRequestData } from 'codify-schemas';
+import { PlanRequestData, ResourceOperation, ValidateResponseData } from 'codify-schemas';
 
 import { PluginValidationError, PluginValidationErrorParams, TypeNotFoundError } from '../common/errors.js';
 import { ctx } from '../events/context.js';
@@ -7,6 +7,7 @@ import { DependencyMap } from '../plugins/plugin-manager.js';
 import { DependencyGraphResolver } from '../utils/dependency-graph-resolver.js';
 import { groupBy } from '../utils/index.js';
 import { ConfigBlock, ConfigType } from './config.js';
+import { type Plan } from './plan.js';
 import { ProjectConfig } from './project-config.js';
 import { ResourceConfig } from './resource-config.js';
 
@@ -125,6 +126,10 @@ ${JSON.stringify(projectConfigs, null, 2)}`);
     this.resourceConfigs.unshift(new ResourceConfig({
       type: 'xcode-tools'
     }));
+
+    if (this.evaluationOrder) {
+      this.evaluationOrder.unshift('xcode-tools');
+    }
   }
 
   validateTypeIds(resourceMap: Map<string, string[]>) {
@@ -135,26 +140,9 @@ ${JSON.stringify(projectConfigs, null, 2)}`);
     }
   }
 
-  resolveResourceDependencies(dependencyMap: DependencyMap) {
-    const resourceMap = new Map(this.resourceConfigs.map((r) => [r.id, r] as const));
-
-    for (const r of this.resourceConfigs) {
-      // User specified dependencies are hard dependencies. They must be present.
-      r.addDependenciesFromDependsOn((id) => resourceMap.has(id));
-      r.addDependenciesBasedOnParameters((id) => resourceMap.has(id));
-
-      // Plugin dependencies are soft dependencies. They only activate if the dependent resource is present.
-      r.addDependencies(dependencyMap.get(r.type)
-        ?.filter((type) => [...resourceMap.values()].some((r) => r.type === type))
-        ?.flatMap((type) => [...resourceMap.values()].filter((r) => r.type === type).map((r) => r.id)) ?? []
-      );
-
-      // Add this to ensure that the default config xcode-tools gets applied first
-      // TODO: remove this in the future with required dependencies
-      if (r.type !== 'xcode-tools') {
-        r.addDependencies(['xcode-tools'])
-      }
-    }
+  resolveDependenciesAndCalculateEvalOrder(dependencyMap?: DependencyMap) {
+    this.resolveResourceDependencies(dependencyMap);
+    this.calculateEvaluationOrder();
   }
 
   handlePluginResourceValidationResults(results: ValidateResponseData[]) {
@@ -172,7 +160,29 @@ ${JSON.stringify(projectConfigs, null, 2)}`);
     }
   }
 
-  calculateEvaluationOrder() {
+  removeNoopFromEvaluationOrder(plan: Plan) {
+    this.evaluationOrder = this.evaluationOrder?.filter((id) =>
+      plan.getResourcePlan(id)?.operation !== ResourceOperation.NOOP,
+    ) ?? null;
+  }
+
+  private resolveResourceDependencies(dependencyMap?: DependencyMap) {
+    const resourceMap = new Map(this.resourceConfigs.map((r) => [r.id, r] as const));
+
+    for (const r of this.resourceConfigs) {
+      // User specified dependencies are hard dependencies. They must be present.
+      r.addDependenciesFromDependsOn((id) => resourceMap.has(id));
+      r.addDependenciesBasedOnParameters((id) => resourceMap.has(id));
+
+      // Plugin dependencies are soft dependencies. They only activate if the dependent resource is present.
+      r.addDependencies(dependencyMap?.get(r.type)
+        ?.filter((type) => [...resourceMap.values()].some((r) => r.type === type))
+        ?.flatMap((type) => [...resourceMap.values()].filter((r) => r.type === type).map((r) => r.id)) ?? []
+      );
+    }
+  }
+
+  private calculateEvaluationOrder() {
     const resourceOrder = DependencyGraphResolver.calculateDependencyList(
       this.resourceConfigs,
       (r) => r.id,

src/orchestrators/destroy.ts

@@ -9,7 +9,7 @@ import { InitializeOrchestrator } from './initialize.js';
 
 export interface DestroyArgs {
   ids: string[];
-  path: string;
+  path?: string;
   secureMode?: boolean;
 }
 
@@ -45,8 +45,7 @@ export class DestroyOrchestrator {
     await DestroyOrchestrator.validate(project, pluginManager, dependencyMap)
 
     const uninstallProject = project.toDestroyProject()
-    uninstallProject.resolveResourceDependencies(dependencyMap);
-    uninstallProject.calculateEvaluationOrder();
+    uninstallProject.resolveDependenciesAndCalculateEvalOrder(dependencyMap);
 
     const plan = await ctx.process(ProcessName.PLAN, () =>
       pluginManager.getPlan(uninstallProject)

src/orchestrators/plan.ts

@@ -23,20 +23,17 @@ export class PlanOrchestrator {
 
     const { dependencyMap, pluginManager, project } = await InitializeOrchestrator.run({
       ...args,
-      transformProject(project) {
-        project.addXCodeToolsConfig();
-        return project;
-      }
     }, reporter);
 
     await createStartupShellScriptsIfNotExists();
 
     await PlanOrchestrator.validate(project, pluginManager, dependencyMap)
+    project.resolveDependenciesAndCalculateEvalOrder(dependencyMap);
+    project.addXCodeToolsConfig(); // We have to add xcode-tools config always since almost every resource depends on it
 
-    project.resolveResourceDependencies(dependencyMap);
-    project.calculateEvaluationOrder();
-
-    const plan = await PlanOrchestrator.plan(project, pluginManager)
+    const plan = await PlanOrchestrator.plan(project, pluginManager);
+    plan.sortByEvalOrder(project.evaluationOrder);
+    project.removeNoopFromEvaluationOrder(plan);
 
     ctx.processFinished(ProcessName.PLAN)
 

src/plugins/plugin-manager.ts

@@ -102,17 +102,18 @@ export class PluginManager {
   }
 
   async apply(project: Project, plan: Plan): Promise<void> {
-    for (const resourcePlan of plan) {
-      ctx.subprocessStarted(SubProcessName.APPLYING_RESOURCE, resourcePlan.id);
+    for (const id of project.evaluationOrder ?? []) {
+      ctx.subprocessStarted(SubProcessName.APPLYING_RESOURCE, id);
 
-      const planRequest = project.getPlanRequest(resourcePlan.id)
-      if (!planRequest) {
-        throw new InternalError(`Could not find plan request: ${resourcePlan.id}`)
+      const resourcePlan = plan.getResourcePlan(id);
+      if (!resourcePlan) {
+        throw new InternalError(`Could not find resourcePlan: ${id}`)
       }
 
-      const pluginName = this.resourceToPluginMapping.get(resourcePlan.resourceType);
+      const { resourceType } = resourcePlan;
+      const pluginName = this.resourceToPluginMapping.get(resourceType);
       if (!pluginName) {
-        throw new InternalError(`Unable to determine plugin for apply: ${resourcePlan.resourceType}`);
+        throw new InternalError(`Unable to determine plugin for apply: ${resourceType}`);
       }
 
       await this.plugins.get(pluginName)!.apply(resourcePlan);

src/utils/index.ts

@@ -19,3 +19,7 @@ export function getTypeAndNameFromId(id: string): { type: string; name: string |
     name: nameParts.length === 0 ? undefined : nameParts.join('.')
   }
 }
+
+export function getId(type: string, name?: string): string {
+  return name ? `${type}.${name}` : type;
+}

test/orchestrator/apply/apply.test.ts

@@ -14,7 +14,7 @@ vi.mock('../../../src/plugins/plugin.js', async () => {
 })
 
 // The apply orchestrator directly calls plan so this will test both
-describe('Apply and plan orchestrator tests', () => {
+describe('Apply orchestrator tests', () => {
   it('Can apply a resource (create)', async () => {
     const reporter = new MockReporter({
       validatePlan(plan: Plan) {

test/orchestrator/plan/create-xcode-tools.codify.json

@@ -0,0 +1,9 @@
+[
+  { "type": "project", "plugins": { "default": "./test/orchestrator/mocks/plugin.ts" } },
+  { "type":  "mock", "propA": "abc", "propB": 123, "directory": "/home" },
+  { "type":  "mock", "propA": "abc", "propB": 123, "directory": "/home" },
+  { "type":  "mock", "propA": "abc", "propB": 123, "directory": "/home" },
+  { "type":  "mock", "propA": "abc", "propB": 123, "directory": "/home" },
+  { "type":  "mock", "propA": "abc", "propB": 123, "directory": "/home" },
+  { "type":  "mock", "propA": "abc", "propB": 123, "directory": "/home" }
+]