Claude.ai #39

moelasmar · 2024-10-28T19:20:48Z

Issue # (if applicable)

Closes #.

Reason for this change

Description of changes

Description of how you validated changes

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Prompt= "I do not think this L2 is following CDK coding guidelines. It missed some properties. Also, for constructs defined in cdk-lib, we do not import from the dependencies, but instead we depend on the modules defined in the project. We shuld use enum where it make sense, as it is a mandatory property. E.g. "engine" property. Given this, can you update the code for elasticache ?"

Prompt: I would like help to create a CDK L2 for Elasticache Claude: I'll help you create a CDK L2 construct for ElastiCache. L2 constructs provide a higher-level abstraction with convenient defaults and better type safety compared to L1 constructs. Let's create one that encapsulates common ElastiCache configuration patterns. This L2 construct provides a higher-level abstraction for creating an ElastiCache cluster. Here's how to use it: Key features of this L2 construct: Sensible defaults for common properties Automatic creation of subnet groups and security groups Optional parameter group creation Helper methods for managing security group rules Easy access to cluster endpoints Proper typing and documentation The construct handles: VPC placement Security group configuration Subnet group creation Parameter group configuration (optional) Engine version management Node type and count configuration Maintenance window configuration Port configuration Would you like me to: Add support for additional features like encryption, backup configuration, or tags? Add methods for scaling or modifying the cluster? Include support for replication groups? Add more configuration options?

moelasmar · 2024-10-30T13:09:50Z

packages/aws-cdk-lib/aws-elasticache/lib/elasticache-cluster.ts

+import * as cdk from 'aws-cdk-lib';
+import * as elasticache from 'aws-cdk-lib/aws-elasticache';
+import * as ec2 from 'aws-cdk-lib/aws-ec2';


for the constructs defined in cdk-lib, we do not import it from the dependencies, but instead we depend on the modules defined in the project. For example,
import * as ec2 from '../../aws-ec2';

moelasmar · 2024-10-30T13:14:20Z

packages/aws-cdk-lib/aws-elasticache/lib/elasticache-cluster.ts

+   * The node type for the ElastiCache cluster
+   * @default - cache.t3.micro
+   */
+  readonly nodeType?: string;


Since there is a list of allowed values, so it should be of enum type.

moelasmar · 2024-10-30T13:15:23Z

packages/aws-cdk-lib/aws-elasticache/lib/elasticache-cluster.ts

+   * Security groups to attach to the cluster
+   * @default - A new security group is created
+   */
+  readonly securityGroups?: ec2.ISecurityGroup[];


there are 2 properties that I found them similar to this property (CacheSecurityGroupNames, VpcSecurityGroupIds), so I am not sure which one it is referring to.

Also, not sure why this property was created as list, although in the code it uses only the first element

moelasmar · 2024-10-30T13:20:07Z

packages/aws-cdk-lib/aws-elasticache/lib/elasticache-cluster.ts

+  /**
+   * The name of the cache parameter group to associate with this cache cluster
+   */
+  readonly parameterGroupFamily?: string;


it seems it is referring to this property. Since there is a list of allowed values, so it should be of enum type.

moelasmar · 2024-10-30T13:20:48Z

packages/aws-cdk-lib/aws-elasticache/lib/elasticache-cluster.ts

+   * The weekly time range during which maintenance updates can occur.
+   * @default - No preference
+   */
+  readonly preferredMaintenanceWindow?: string;


Since there is an allowed format for this property, in CDK we usually create a new Type that simplify it. For example this property can of a new Type which is:

interface DayTime { day Enum; time number; // we can add a validation that it is between 0-23 } interface MaintenanceWindow { startMaintenanceWindow DayTime; endMaintenanceWindow DayTime; // we can add a validation that the end maintenance window is after the start }

moelasmar · 2024-10-30T13:22:29Z

packages/aws-cdk-lib/aws-elasticache/lib/elasticache-cluster.ts

+import * as ec2 from 'aws-cdk-lib/aws-ec2';
+import { Construct } from 'constructs';
+
+export interface ElastiCacheClusterProps {


there are many missing properties:

clusterName

engine (mandatory)

subnetGroup

autoMinorVersionUpgrade

snapshotRetentionLimit

snapshotWindow

SnapshotName

PreferredAvailabilityZones

IpDiscovery

moelasmar · 2024-10-30T13:24:34Z

packages/aws-cdk-lib/aws-elasticache/lib/elasticache-cluster.ts

+   * The cache engine version to use
+   * @default - 6.x (Redis)
+   */
+  readonly engineVersion?: string;


this property has a relation with the missed property engine .. may be the suggestion here is to replace the 2 properties (engine and version) with this property, but this is not clear from comment, and also IMOP, it should accept a limited list of values, so I believe it should be enum.

moelasmar · 2024-10-30T13:25:40Z

packages/aws-cdk-lib/aws-elasticache/lib/elasticache-cluster.ts

+  readonly port?: number;
+}
+
+export class ElastiCacheCluster extends Construct {


this construct is missing the construct interface. You can find more details here

This construct also some of the important patterns we usually have in CDK:

Imports, which means to have static methods that customer can use them to create an object from this construct for a resource that already exist in their account. Check here from more details

Grants, which are the methods that can grant specific role or use to do some actions on the ElastiCacheCluster. Check here for more details

Since this construct use the EC2 security groups, so it should implement Ec2 Connections API. Check here for details

moelasmar · 2024-10-30T13:33:17Z

packages/aws-cdk-lib/aws-elasticache/lib/elasticache-cluster.ts

+
+    // Create the ElastiCache cluster
+    this.cluster = new elasticache.CfnCacheCluster(this, 'Resource', {
+      engine: 'redis',


it totally ignore all other 2 engines

moelasmar · 2024-10-30T13:33:41Z

packages/aws-cdk-lib/aws-elasticache/lib/elasticache-cluster.ts

+      cacheNodeType: props.nodeType ?? 'cache.t3.micro',
+      numCacheNodes: props.numCacheNodes ?? 1,


these default values are not best practices, or at least in the service console, we suggest 3 predefined types of Clusters (Production, Dev/test, demo), and each type has some default values. We should replicate the same concept in CDK.

moelasmar · 2024-10-30T13:35:57Z

similar to Q one, I have the same comment on the constructs design, as in general, I will use a different design for this construct.

<<Interface>> IElastiCacheCluster
|
|___ <<Class >> RedisElastiCacheCluster
|      |
|      |___ <<Class >> ProductionRedisElastiCacheCluster
|      |
|      |___ <<Class >> DevTestRedisElastiCacheCluster
|      |
|      |___ <<Class >> DemoRedisElastiCacheCluster
|
|___ <<Class >> ValKeyElastiCacheCluster
|      |
|      |___ <<Class >> ProductionValKeyElastiCacheCluster
|      |
|      |___ <<Class >> DevTestValKeyElastiCacheCluster
|      |
|      |___ <<Class >> DemoValKeyElastiCacheCluster
|
|___ <<Class >> MemcachedElastiCacheCluster
       |
       |___ <<Class >> ProductionMemcachedElastiCacheCluster
       |
       |___ <<Class >> DevTestMemcachedElastiCacheCluster
       |
       |___ <<Class >> DemoMemcachedElastiCacheCluster

moelasmar · 2024-10-30T13:52:07Z

packages/aws-cdk-lib/aws-elasticache/lib/elasticache-cluster.ts

+  public allowIngressFrom(peer: ec2.IPeer, port?: number) {
+    this.securityGroup.addIngressRule(
+      peer,
+      ec2.Port.tcp(port ?? 6379),


it added the default port here, but it did not use it while creating the CfnCacheCluster resource

kathykam added 6 commits October 9, 2024 01:11

Q generated Elasticache L2

2a929c0

Q generated with @workspace context

67d5c67

Merge branch 'aws:main' into main

a7300c9

Merge branch 'aws:main' into claude.ai

beaa31c

moelasmar marked this pull request as draft October 28, 2024 19:20

github-actions bot added the p2 label Oct 28, 2024

moelasmar commented Oct 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Claude.ai #39

Claude.ai #39

moelasmar commented Oct 28, 2024

moelasmar Oct 30, 2024

moelasmar Oct 30, 2024

moelasmar Oct 30, 2024

moelasmar Oct 30, 2024

moelasmar Oct 30, 2024

moelasmar Oct 30, 2024

moelasmar Oct 30, 2024

moelasmar Oct 30, 2024

moelasmar Oct 30, 2024

moelasmar Oct 30, 2024

moelasmar Oct 30, 2024 •

edited

Loading

moelasmar Oct 30, 2024

moelasmar commented Oct 30, 2024

moelasmar Oct 30, 2024

		cacheNodeType: props.nodeType ?? 'cache.t3.micro',
		numCacheNodes: props.numCacheNodes ?? 1,

Claude.ai #39

Are you sure you want to change the base?

Claude.ai #39

Conversation

moelasmar commented Oct 28, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moelasmar Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moelasmar commented Oct 30, 2024

Choose a reason for hiding this comment

moelasmar Oct 30, 2024 •

edited

Loading