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

refactor: Encapsulate overriding of logicalId in a single place #364

Merged
merged 1 commit into from
Apr 7, 2021

Commits on Apr 7, 2021

  1. refactor: Encapsulate overriding of logicalId in a single place

    A resource's logicalId maps to the name of the created resource.
    
    For example, take this CloudFormation YAML for an ASG:
    
    ```yaml
    DataNodeAutoScalingGroup:
      Type: AWS::AutoScaling::AutoScalingGroup
      Properties:
        AvailabilityZones: !GetAZs ""
        LaunchConfigurationName: !Ref DataNodeLaunchConfig
        MinSize: 3
        MaxSize: 4
        Cooldown: 180
        HealthCheckType: EC2
        HealthCheckGracePeriod: 300
        VPCZoneIdentifier: !Ref PrivateVpcSubnets
        Tags:
        - { Key: Stack, Value: !Ref Stack, PropagateAtLaunch: true }
        - { Key: App, Value: "elk-es-data", PropagateAtLaunch: true }
        - { Key: Stage, Value: !Ref Stage, PropagateAtLaunch: true }
        - { Key: ElasticSearchCluster, Value: !Ref ClusterName, PropagateAtLaunch: true }
    ```
    
    This creates an ASG called `DataNodeAutoScalingGroup`. This pattern is widely used in YAML definitions of CloudFormation as YAML files are typically hand-written.
    
    CDK changes this as it auto-generates the logicalIds, as there one will gain more understanding about the resource from looking at the CDK template.
    
    In order to enable a no-op migration between a YAML stack a CDK stack, we allow the logicalId to be overridden.
    Our constructs seem to follow different rules regarding when to override the logicalId of a CloudFormation resource.
    
    For example, we have [this](https://github.com/guardian/cdk/blob/60cbf21698b3ddbb8cfd23556fd2af886e9551c7/src/constructs/rds/instance.ts#L38-L40):
    
    ```typescript
    if (props.overrideId || (scope.migratedFromCloudFormation && props.overrideId !== false))
    ```
    
    And [this](https://github.com/guardian/cdk/blob/60cbf21698b3ddbb8cfd23556fd2af886e9551c7/src/constructs/iam/policies/base-policy.ts#L15-L18):
    
    ```typescript
    if (props.overrideId) {
    ```
    
    This change aims to bring consistency and simplicity across our constructs by applying a universal rule that,
    `migratedFromCloudFormation` on the `GuStack` must be `true` and `existingLogicalId` to be set for the logicalId to be overridden.
    
    If these conditions are not met, we let CDK auto-generate the logicalId.
    
    It's worth noting that this doesn't implement the change on the constructs, it's simply to define the behaviour.
    This is mainly to keep the diff down - we'll migrate the constructs in follow-up PRs.
    
    Usage will roughly look like this:
    
    ```typescript
    const MyComponentProps extends GuMigratingResource { }
    
    class MyComponent extends SomeGuConstruct {
      constructor(scope: GuStack, id: string, props: MyComponentProps) {
        super(scope, id, props);
        GuMigratingResource.setLogicalId(this, scope, props);
      }
    }
    ```
    akash1810 committed Apr 7, 2021
    Configuration menu
    Copy the full SHA
    5274685 View commit details
    Browse the repository at this point in the history