Addressed review comments

Author: Tarun Belani

packages/@aws-cdk/aws-imagebuilder-alpha/README.md

@@ -133,7 +133,7 @@ const advancedSchedulePipeline = new imagebuilder.ImagePipeline(this, 'AdvancedS
     autoDisableFailureCount: 3
   },
   // Start enabled
-  enabled: true
+  status: imagebuilder.ImagePipelineStatus.ENABLED
 });
 ```
 

packages/@aws-cdk/aws-imagebuilder-alpha/lib/image-pipeline.ts

@@ -132,9 +132,9 @@ export interface ImagePipelineProps {
   /**
    * Indicates whether the pipeline is enabled to be triggered by the provided schedule
    *
-   * @default true
+   * @default ImagePipelineStatus.ENABLED
    */
-  readonly enabled?: boolean;
+  readonly status?: ImagePipelineStatus;
 
   /**
    * The infrastructure configuration used for building the image.
@@ -278,6 +278,21 @@ export enum ScheduleStartCondition {
   EXPRESSION_MATCH_AND_DEPENDENCY_UPDATES_AVAILABLE = 'EXPRESSION_MATCH_AND_DEPENDENCY_UPDATES_AVAILABLE',
 }
 
+/**
+ * Indicates whether the pipeline is enabled to be triggered by the provided schedule
+ */
+export enum ImagePipelineStatus {
+  /**
+   * Indicates that the pipeline is enabled for scheduling
+   */
+  ENABLED = 'ENABLED',
+
+  /**
+   * Indicates that the pipeline is disabled and will not be triggered on the schedule
+   */
+  DISABLED = 'DISABLED',
+}
+
 /**
  * A new or imported Image Pipeline
  */
@@ -491,21 +506,35 @@ export class ImagePipeline extends ImagePipelineBase {
       this.infrastructureConfiguration._bind({ isContainerBuild: props.recipe._isContainerRecipe() });
     }
 
+    if (props.recipe._isImageRecipe() && props.recipe._isContainerRecipe()) {
+      throw new cdk.ValidationError('the recipe must either be an IImageRecipe or IContainerRecipe', this);
+    }
+
+    if (!props.recipe._isImageRecipe() && !props.recipe._isContainerRecipe()) {
+      throw new cdk.ValidationError('the recipe must either be an IImageRecipe or IContainerRecipe', this);
+    }
+
     const imagePipeline = new CfnImagePipeline(this, 'Resource', {
       name: this.physicalName,
       description: props.description,
       ...(props.recipe._isImageRecipe() && { imageRecipeArn: props.recipe.imageRecipeArn }),
       ...(props.recipe._isContainerRecipe() && { containerRecipeArn: props.recipe.containerRecipeArn }),
-      ...(props.enabled !== undefined && { status: props.enabled ? 'ENABLED' : 'DISABLED' }),
+      ...(props.status !== undefined && { status: props.status }),
       infrastructureConfigurationArn: this.infrastructureConfiguration.infrastructureConfigurationArn,
       distributionConfigurationArn: props.distributionConfiguration?.distributionConfigurationArn,
       enhancedImageMetadataEnabled: props.enhancedImageMetadataEnabled,
       executionRole: this.executionRole?.roleArn,
       schedule: this.buildSchedule(props),
       loggingConfiguration: this.buildLoggingConfiguration(props),
-      imageTestsConfiguration: buildImageTestsConfiguration(props),
-      imageScanningConfiguration: buildImageScanningConfiguration(scope, props),
-      workflows: buildWorkflows(props),
+      imageTestsConfiguration: buildImageTestsConfiguration<
+        ImagePipelineProps,
+        CfnImagePipeline.ImageTestsConfigurationProperty
+      >(props),
+      imageScanningConfiguration: buildImageScanningConfiguration<
+        ImagePipelineProps,
+        CfnImagePipeline.ImageScanningConfigurationProperty
+      >(this, props),
+      workflows: buildWorkflows<ImagePipelineProps, CfnImagePipeline.WorkflowConfigurationProperty[]>(props),
       tags: props.tags,
     });
 

packages/@aws-cdk/aws-imagebuilder-alpha/lib/private/image-and-pipeline-props-helper.ts

@@ -1,99 +1,77 @@
 import * as cdk from 'aws-cdk-lib';
-import * as ecr from 'aws-cdk-lib/aws-ecr';
-import { CfnImagePipeline } from 'aws-cdk-lib/aws-imagebuilder';
+import { CfnImage, CfnImagePipeline } from 'aws-cdk-lib/aws-imagebuilder';
 import { Construct } from 'constructs';
-import { IRecipeBase } from '../recipe-base';
-import { WorkflowConfiguration, WorkflowOnFailure } from '../workflow';
+import { ImagePipelineProps } from '../image-pipeline';
 
 /**
  * Generates the image tests configuration property into the `ImageTestsConfiguration` type in the CloudFormation L1
  * definition.
  *
- * @param imageTestsEnabled Props input for whether image tests are enabled
+ * @param props Props input for the construct
  */
-export const buildImageTestsConfiguration = ({
-  imageTestsEnabled,
-}: {
-  imageTestsEnabled?: boolean;
-}): { imageTestsEnabled: boolean } | undefined => {
-  if (imageTestsEnabled === undefined) {
+export const buildImageTestsConfiguration = <
+  PropsT extends ImagePipelineProps,
+  OutputT extends CfnImagePipeline.ImageTestsConfigurationProperty | CfnImage.ImageTestsConfigurationProperty,
+>(
+  props: PropsT,
+): OutputT | undefined => {
+  if (props.imageTestsEnabled === undefined) {
     return undefined;
   }
 
-  return { imageTestsEnabled };
+  return { imageTestsEnabled: props.imageTestsEnabled } as OutputT;
 };
 
 /**
  * Generates the image scanning configuration property into the `ImageScanningConfiguration` type in the CloudFormation
  * L1 definition.
  *
  * @param scope The construct scope
- * @param imageScanningEnabled Props input for whether image scanning is enabled
- * @param imageScanningEcrRepository Props input for the image scanning ECR repository
- * @param imageScanningEcrTags Props input for the image scanning ECR tags
- * @param recipe Props input for the image or container recipe
+ * @param props Props input for the construct
  */
-export const buildImageScanningConfiguration = (
+export const buildImageScanningConfiguration = <
+  PropsT extends ImagePipelineProps,
+  OutputT extends CfnImagePipeline.ImageScanningConfigurationProperty | CfnImage.ImageScanningConfigurationProperty,
+>(
   scope: Construct,
-  {
-    imageScanningEnabled,
-    imageScanningEcrRepository,
-    imageScanningEcrTags,
-    recipe,
-  }: {
-    imageScanningEnabled?: boolean;
-    imageScanningEcrRepository?: ecr.IRepository;
-    imageScanningEcrTags?: string[];
-    recipe: IRecipeBase;
-  },
-):
-  | { ecrConfiguration?: { containerTags?: string[]; repositoryName?: string }; imageScanningEnabled?: boolean }
-  | undefined => {
-  if (!recipe._isContainerRecipe() && imageScanningEcrRepository !== undefined) {
+  props: PropsT,
+): OutputT | undefined => {
+  if (!props.recipe._isContainerRecipe() && props.imageScanningEcrRepository !== undefined) {
     throw new cdk.ValidationError('imageScanningEcrRepository is only supported for container recipe builds', scope);
   }
 
-  if (!recipe._isContainerRecipe() && imageScanningEcrTags !== undefined) {
+  if (!props.recipe._isContainerRecipe() && props.imageScanningEcrTags !== undefined) {
     throw new cdk.ValidationError('imageScanningEcrTags is only supported for container recipe builds', scope);
   }
 
   const ecrConfiguration: CfnImagePipeline.EcrConfigurationProperty = {
-    ...(imageScanningEcrRepository !== undefined && {
-      repositoryName: imageScanningEcrRepository.repositoryName,
+    ...(props.imageScanningEcrRepository !== undefined && {
+      repositoryName: props.imageScanningEcrRepository.repositoryName,
     }),
-    ...((imageScanningEcrTags ?? []).length && { containerTags: imageScanningEcrTags }),
+    ...((props.imageScanningEcrTags ?? []).length && { containerTags: props.imageScanningEcrTags }),
   };
 
   const imageScanningConfiguration = {
-    ...(imageScanningEnabled !== undefined && { imageScanningEnabled: imageScanningEnabled }),
+    ...(props.imageScanningEnabled !== undefined && { imageScanningEnabled: props.imageScanningEnabled }),
     ...(Object.keys(ecrConfiguration).length && { ecrConfiguration }),
   };
 
-  return Object.keys(imageScanningConfiguration).length ? imageScanningConfiguration : undefined;
+  return Object.keys(imageScanningConfiguration).length ? (imageScanningConfiguration as OutputT) : undefined;
 };
 
 /**
  * Generates the workflows property into the `WorkflowConfiguration` type in the CloudFormation L1
  * definition.
  *
- * @param workflows Props input for the workflows
+ * @param props Props input for the construct
  */
-export const buildWorkflows = ({
-  workflows,
-}: {
-  workflows?: WorkflowConfiguration[];
-}):
-  | {
-    workflowArn: string;
-    onFailure?: WorkflowOnFailure;
-    parallelGroup?: string;
-    parameters?: {
-      name: string;
-      value: string[];
-    }[];
-  }[]
-  | undefined => {
-  const cfnWorkflows = workflows?.map((workflow) => {
+export const buildWorkflows = <
+  PropsT extends ImagePipelineProps,
+  OutputT extends CfnImagePipeline.WorkflowConfigurationProperty[] | CfnImage.WorkflowConfigurationProperty[],
+>(
+  props: PropsT,
+): OutputT | undefined => {
+  const cfnWorkflows = props.workflows?.map((workflow) => {
     const parameters = Object.entries(workflow.parameters ?? {}).map(([name, value]) => ({
       name,
       value: value.value,
@@ -106,5 +84,5 @@ export const buildWorkflows = ({
     };
   });
 
-  return cfnWorkflows?.length ? cfnWorkflows : undefined;
+  return cfnWorkflows?.length ? (cfnWorkflows as OutputT) : undefined;
 };

packages/@aws-cdk/aws-imagebuilder-alpha/test/image-pipeline.test.ts

@@ -8,9 +8,13 @@ import {
   AwsManagedWorkflow,
   ContainerRecipe,
   DistributionConfiguration,
+  IContainerRecipe,
+  IImageRecipe,
   ImagePipeline,
+  ImagePipelineStatus,
   ImageRecipe,
   InfrastructureConfiguration,
+  IRecipeBase,
   ScheduleStartCondition,
   WorkflowOnFailure,
   WorkflowParameterValue,
@@ -97,7 +101,7 @@ describe('ImagePipeline', () => {
         'DistributionConfiguration',
         'imported-distribution-configuration',
       ),
-      enabled: true,
+      status: ImagePipelineStatus.ENABLED,
       executionRole: iam.Role.fromRoleName(stack, 'ExecutionRole', 'imagebuilder-execution-role'),
       schedule: {
         expression: events.Schedule.cron({ minute: '0', hour: '0', weekDay: '0' }),
@@ -217,7 +221,7 @@ describe('ImagePipeline', () => {
         'imported-distribution-configuration',
       ),
       executionRole: iam.Role.fromRoleName(stack, 'ExecutionRole', 'imagebuilder-execution-role'),
-      enabled: false,
+      status: ImagePipelineStatus.DISABLED,
       schedule: {
         expression: events.Schedule.rate(cdk.Duration.days(7)),
         startCondition: ScheduleStartCondition.EXPRESSION_MATCH_AND_DEPENDENCY_UPDATES_AVAILABLE,
@@ -880,4 +884,62 @@ describe('ImagePipeline', () => {
       });
     }).toThrow(cdk.ValidationError);
   });
+
+  test('throws a validation error when the recipe is neither an IImageRecipe or IContainerRecipe', () => {
+    class BadRecipe extends cdk.Resource implements IRecipeBase {
+      public grant(grantee: iam.IGrantable, ...actions: string[]): iam.Grant {
+        return iam.Grant.addToPrincipal({
+          grantee,
+          actions,
+          resourceArns: ['*'],
+          scope: stack,
+        });
+      }
+
+      public grantRead(grantee: iam.IGrantable): iam.Grant {
+        return this.grant(grantee, 'imagebuilder:GetBadRecipe');
+      }
+
+      public _isContainerRecipe(): this is IContainerRecipe {
+        return false;
+      }
+
+      public _isImageRecipe(): this is IImageRecipe {
+        return false;
+      }
+    }
+
+    expect(() => new ImagePipeline(stack, 'ImagePipeline', { recipe: new BadRecipe(stack, 'BadRecipe') })).toThrow(
+      cdk.ValidationError,
+    );
+  });
+
+  test('throws a validation error when the recipe is both an IImageRecipe and IContainerRecipe', () => {
+    class BadRecipe extends cdk.Resource implements IRecipeBase {
+      public grant(grantee: iam.IGrantable, ...actions: string[]): iam.Grant {
+        return iam.Grant.addToPrincipal({
+          grantee,
+          actions,
+          resourceArns: ['*'],
+          scope: stack,
+        });
+      }
+
+      public grantRead(grantee: iam.IGrantable): iam.Grant {
+        return this.grant(grantee, 'imagebuilder:GetBadRecipe');
+      }
+
+      public _isContainerRecipe(): this is IContainerRecipe {
+        return true;
+      }
+
+      public _isImageRecipe(): this is IImageRecipe {
+        return true;
+      }
+    }
+
+    expect(() => new ImagePipeline(stack, 'ImagePipeline', { recipe: new BadRecipe(stack, 'BadRecipe') })).toThrow(
+      cdk.ValidationError,
+    );
+  });
 });

packages/@aws-cdk/aws-imagebuilder-alpha/test/integ.all-parameters.image-pipeline.ami.ts

@@ -32,7 +32,7 @@ const imagePipeline = new imagebuilder.ImagePipeline(stack, 'ImagePipeline-AMI',
   recipe: imageRecipe,
   infrastructureConfiguration,
   distributionConfiguration: amiDistributionConfiguration,
-  enabled: true,
+  status: imagebuilder.ImagePipelineStatus.ENABLED,
   executionRole,
   schedule: {
     expression: events.Schedule.expression('cron(0 7 ? * mon *)'),

packages/@aws-cdk/aws-imagebuilder-alpha/test/integ.all-parameters.image-pipeline.container.js.snapshot/aws-cdk-imagebuilder-image-pipeline-container-all-parameters.assets.json

@@ -799,7 +799,7 @@
      "PipelineExecutionStartCondition": "EXPRESSION_MATCH_AND_DEPENDENCY_UPDATES_AVAILABLE",
      "ScheduleExpression": "rate(7 days)"
     },
-    "Status": "ENABLED",
+    "Status": "DISABLED",
     "Workflows": [
      {
       "WorkflowArn": {

packages/@aws-cdk/aws-imagebuilder-alpha/test/integ.all-parameters.image-pipeline.container.js.snapshot/aws-cdk-imagebuilder-image-pipeline-container-all-parameters.template.json

@@ -38,7 +38,7 @@ const containerImagePipeline = new imagebuilder.ImagePipeline(stack, 'ImagePipel
   recipe: containerRecipe,
   infrastructureConfiguration,
   distributionConfiguration: containerDistributionConfiguration,
-  enabled: true,
+  status: imagebuilder.ImagePipelineStatus.DISABLED,
   executionRole,
   schedule: {
     expression: events.Schedule.rate(cdk.Duration.days(7)),