Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Introduce mixins for migratable constructs #418

Merged
merged 3 commits into from
Apr 12, 2021

Conversation

akash1810
Copy link
Member

@akash1810 akash1810 commented Apr 12, 2021

What does this change?

Currently, to define a migratable construct you need to:

  • ensure the props implement the correct interfaces
  • call GuMigratingResource.setLogicalId in the constructor

This is quite a lot and if there are no tests against the latter, we're just hoping to spot mistakes at review time.

Enter mixins!

Along with traditional OO hierarchies, another popular way of building up classes from reusable components is to build them by combining simpler partial classes. You may be familiar with the idea of mixins or traits for languages like Scala, and the pattern has also reached some popularity in the JavaScript community.

In this change, we create two mixins:

  1. GuMigratableConstruct
  2. GuStatefulMigratableConstruct

These mixins greatly simplify the process of defining a migratable construct.

For example, for a stateful construct, we go from:

class GuMyConstruct extends AwsConstruct implements GuStatefulConstruct {
  isStatefulConstruct: true;

  constructor(scope: GuStack, id: string, props: GuMyConstructProps) {
    super(scope, id, props);

    this.isStatefulConstruct = true;

    GuMigratingResource.setLogicalId(
      this,
      {
        migratedFromCloudFormation: scope.migratedFromCloudFormation
      },
      {
        existingLogicalId: props.existingLogicalId
      }
    );
  }
}

To:

class GuMyConstruct extends GuStatefulMigratableConstruct(AwsConstruct) { }

There is a reduction of boilerplate and less need to explicitly test the migration logic at each individual construct level. Thus less reliance on code review catching mistakes.

This should also make the adoption (#400) of the revised logicalId logic (#364) much simpler.

Lastly, to demonstrate the mixin, the only construct using the new logic (GuCertificate) has been refactored. This is in its own commit, happy to make it it's own PR for true separation of concerns.

Does this change require changes to existing projects or CDK CLI?

No.

How to test

New tests added. Look at CI to see them and existing tests pass.

How can we measure success?

  • A simpler API to follow
  • Less need to explicitly test the migration logic at each individual construct level

Have we considered potential risks?

Mixins can definitely become complicated. They can also be quite simple and useful and bring a sprinkling of Scala to a TypeScript codebase!

The risk is the codebase is more complicated to understand.

The movement of the GuStatefulConstruct interface from constructs/core/migrating to utils/mixin might make cause a bit of rebasing pain for open PRs.

Having a specific types directory might help here? Something like this:

src
├── constructs
│   └── core
│       └── stack.ts
└── types
    └── constructs
        └── core
            └── stack.ts

Updates the custom linter rule to cope with spread arguments.

For example:

```typescript
class Foo extends Bar {
  constructor(...args: any[]) {

  }
}
```

This is to handle TypeScript mixins.

We're not changing the custom linter rule here, just ensuring it doesn't throw an exception when `param.left` is `undefined`.

See:
  - https://www.typescriptlang.org/docs/handbook/mixins.html
Currently, to define a migratable construct you need to:
  - ensure the props implement the correct interfaces
  - call `GuMigratingResource.setLogicalId` in the constructor

This is quite a lot and if there are no tests against the latter, we're just hoping to spot mistakes at review time.

Enter mixins!

> Along with traditional OO hierarchies, another popular way of building up classes from reusable components is to build them by combining simpler partial classes. You may be familiar with the idea of mixins or traits for languages like Scala, and the pattern has also reached some popularity in the JavaScript community.

In this change, we create two mixins:
  1. GuMigratableConstruct
  2. GuStatefulMigratableConstruct

These mixins greatly simplify the process of defining a migratable construct.

For example, for a stateful construct, we go from:

```typescript
class GuMyConstruct extends AwsConstruct implements GuStatefulConstruct {
  isStatefulConstruct: true;

  constructor(scope: GuStack, id: string, props: GuMyConstructProps) {
    super(scope, id, props);

    this.isStatefulConstruct = true;

    GuMigratingResource.setLogicalId(
      this,
      {
        migratedFromCloudFormation: scope.migratedFromCloudFormation
      },
      {
        existingLogicalId: props.existingLogicalId
      }
    );
  }
}
```

To:

```typescript
class GuMyConstruct extends GuStatefulMigratableConstruct(AwsConstruct) { }
```

There is a reduction of boilerplate and less need to explicitly test the migration logic at each individual construct level. Thus less reliance on code review catching mistakes.

This should also make the adoption (#400) of the revised logicalId logic (#364) much simpler.

See:
  - https://www.typescriptlang.org/docs/handbook/mixins.html
@akash1810 akash1810 requested a review from a team April 12, 2021 06:54
@jacobwinch
Copy link
Contributor

Lastly, to demonstrate the mixin, the only construct using the new logic (GuCertificate) has been refactored. This is in its own commit, happy to make it it's own PR for true separation of concerns.

It's useful to have one example in this PR; it helped me to understand the change properly!

Copy link
Contributor

@jacobwinch jacobwinch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 💯

Although the code is a little more complex, I think it's definitely worthwhile as this approach:

just hoping to spot mistakes at review time.

would definitely have tripped us up at some point!

@akash1810 akash1810 merged commit a1cda09 into main Apr 12, 2021
@akash1810 akash1810 deleted the aa-migratable-construct-mixin branch April 12, 2021 10:40
@github-actions
Copy link
Contributor

🎉 This PR is included in version 8.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

akash1810 added a commit that referenced this pull request Apr 12, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuApplicationListener` construct to adopt the new logic. As of #418 it's as simple as using the `GuStatefulMigratableConstruct` mixin!

The logicalId logic currently in `GuApplicationListener` is very complicated with multiple paths to overriding the logicalId.

This is massively simplified, as evidenced in the updated tests.
akash1810 added a commit that referenced this pull request Apr 12, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuApplicationListener` construct to adopt the new logic. As of #418 it's as simple as using the `GuStatefulMigratableConstruct` mixin!

The logicalId logic currently in `GuApplicationListener` is very complicated with multiple paths to overriding the logicalId.

This is massively simplified, as evidenced in the updated tests.
akash1810 added a commit that referenced this pull request Apr 12, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuPolicy` construct to adopt the new logic. As of #418 it's as simple as using the GuStatefulMigratableConstruct mixin!
akash1810 added a commit that referenced this pull request Apr 12, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuRole` construct to adopt the new logic. As of #418 it's as simple as using the GuStatefulMigratableConstruct mixin!

As a by-product, `GuInstanceRole` also gets updated; the snapshot tests are updated to reflect the fact that `GuInstanceRole` doesn't set `existingLogicalId` when calling `GuRole`, therefore the logicalId will always be autp-generated.
akash1810 added a commit that referenced this pull request Apr 12, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuRole` construct to adopt the new logic. As of #418 it's as simple as using the `GuStatefulMigratableConstruct` mixin!

As a by-product, `GuInstanceRole`'s tests need updating. The snapshot tests are updated to reflect the fact that `GuInstanceRole` doesn't set `existingLogicalId` when calling `GuRole`, therefore the logicalId will always be auto-generated.
akash1810 added a commit that referenced this pull request Apr 12, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuBaseSecurityGroup` construct to adopt the new logic. As of #418 it's as simple as using the `GuStatefulMigratableConstruct` mixin!
akash1810 added a commit that referenced this pull request Apr 12, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuKinesisStream` construct to adopt the new logic. As of #418 it's as simple as using the `GuStatefulMigratableConstruct` mixin!
akash1810 added a commit that referenced this pull request Apr 12, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuBaseSecurityGroup` construct to adopt the new logic. As of #418 it's as simple as using the `GuMigratableConstruct` mixin!
akash1810 added a commit that referenced this pull request Apr 12, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuRole` construct to adopt the new logic. As of #418 it's as simple as using the `GuMigratableConstruct` mixin!

As a by-product, `GuInstanceRole`'s tests need updating. The snapshot tests are updated to reflect the fact that `GuInstanceRole` doesn't set `existingLogicalId` when calling `GuRole`, therefore the logicalId will always be auto-generated.
akash1810 added a commit that referenced this pull request Apr 12, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuPolicy` construct to adopt the new logic. As of #418 it's as simple as using the `GuMigratableConstruct` mixin!
akash1810 added a commit that referenced this pull request Apr 12, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuSnsTopic` construct to adopt the new logic. As of #418 it's as simple as using the `GuStatefulMigratableConstruct` mixin!
akash1810 added a commit that referenced this pull request Apr 12, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuDatabaseInstance` construct to adopt the new logic. As of #418 it's as simple as using the `GuStatefulMigratableConstruct` mixin!
akash1810 added a commit that referenced this pull request Apr 13, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuApplicationTargetGroup` construct to adopt the new logic. As of #418 it's as simple as using the `GuStatefulMigratableConstruct` mixin!
akash1810 added a commit that referenced this pull request Apr 13, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuApplicationLoadBalancer` construct to adopt the new logic. As of #418 it's as simple as using the `GuStatefulMigratableConstruct` mixin!
akash1810 added a commit that referenced this pull request Apr 13, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuClassicLoadBalancer` construct to adopt the new logic. As of #418 it's as simple as using the `GuStatefulMigratableConstruct` mixin!
akash1810 added a commit that referenced this pull request Apr 13, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuAutoScalingGroup` construct to adopt the new logic. As of #418 it's as simple as using the `GuStatefulMigratableConstruct` mixin!
akash1810 added a commit that referenced this pull request Apr 13, 2021
In #364 we placed the logic of overriding a construct's logicalId into a single place.

In this change, we're updating the `GuClassicLoadBalancer` construct to adopt the new logic. As of #418 it's as simple as using the `GuStatefulMigratableConstruct` mixin!
akash1810 added a commit that referenced this pull request Oct 22, 2021
When a construct is app aware we:
  - Suffix it's ID with the app string
  - Add the `App` tag

Currently we have to remember to do this and remember it during review.

This change creates a mixin to simplify this.

Before
```typescript
interface MyBucketProps extends BucketProps, AppIdentity {}

export class MyBucket extends Bucket {
  constructor(scope: GuStack, id: string, props: MyBucketProps) {
    const idWithApp = AppIdentity.suffixText(props, id) # <-- this is getting replaced

    super(scope, idWithApp, props);

    AppIdentity.taggedConstruct(props, this); # <-- this is getting replaced
  }
}
```

After
```typescript
interface MyBucketProps extends BucketProps, AppIdentity {}

class MyBucket extends GuAppAwareConstruct(Bucket) {
  constructor(scope: GuStack, id: string, props: MyBucketProps) {
    super(scope, id, props);
  }
}
```

This is similar to #418.
akash1810 added a commit that referenced this pull request Oct 22, 2021
When a construct is app aware we:
  - Suffix it's ID with the app string
  - Add the `App` tag

Currently we have to remember to do this and remember it during review.

This change creates a mixin to simplify this.

Before
```typescript
interface MyBucketProps extends BucketProps, AppIdentity {}

export class MyBucket extends Bucket {
  constructor(scope: GuStack, id: string, props: MyBucketProps) {
    const idWithApp = AppIdentity.suffixText(props, id) # <-- this is getting replaced

    super(scope, idWithApp, props);

    AppIdentity.taggedConstruct(props, this); # <-- this is getting replaced
  }
}
```

After
```typescript
interface MyBucketProps extends BucketProps, AppIdentity {}

class MyBucket extends GuAppAwareConstruct(Bucket) {
  constructor(scope: GuStack, id: string, props: MyBucketProps) {
    super(scope, id, props);
  }
}
```

This is similar to #418.
akash1810 added a commit that referenced this pull request Oct 22, 2021
When a construct is app aware we:
  - Suffix it's ID with the app string
  - Add the `App` tag

Currently we have to remember to do this and remember it during review.

This change creates a mixin to simplify this.

Before
```typescript
interface MyBucketProps extends BucketProps, AppIdentity {}

export class MyBucket extends Bucket {
  constructor(scope: GuStack, id: string, props: MyBucketProps) {
    const idWithApp = AppIdentity.suffixText(props, id) # <-- this is getting replaced

    super(scope, idWithApp, props);

    AppIdentity.taggedConstruct(props, this); # <-- this is getting replaced
  }
}
```

After
```typescript
interface MyBucketProps extends BucketProps, AppIdentity {}

class MyBucket extends GuAppAwareConstruct(Bucket) {
  constructor(scope: GuStack, id: string, props: MyBucketProps) {
    super(scope, id, props);
  }
}
```

This is similar to #418.
akash1810 added a commit that referenced this pull request Oct 22, 2021
When a construct is app aware we:
  - Suffix it's ID with the app string
  - Add the `App` tag

Currently we have to remember to do this and remember it during review.

This change creates a mixin to simplify this.

Before
```typescript
interface MyBucketProps extends BucketProps, AppIdentity {}

export class MyBucket extends Bucket {
  constructor(scope: GuStack, id: string, props: MyBucketProps) {
    const idWithApp = AppIdentity.suffixText(props, id) # <-- this is getting replaced

    super(scope, idWithApp, props);

    AppIdentity.taggedConstruct(props, this); # <-- this is getting replaced
  }
}
```

After
```typescript
interface MyBucketProps extends BucketProps, AppIdentity {}

class MyBucket extends GuAppAwareConstruct(Bucket) {
  constructor(scope: GuStack, id: string, props: MyBucketProps) {
    super(scope, id, props);
  }
}
```

This is similar to #418.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants