Skip to content

Commit

Permalink
chore(awslint): rules permit constructs to extend from 'constructs' m…
Browse files Browse the repository at this point in the history
…odule (#10472)

Introduce an environment variable - `AWSLINT_BASE_CONSTRUCT`
recognized by `awslint`. This environment variable indicates that the
module has [migrated][compat-rfc] away from construct classes and
interfaces from `@aws-cdk/core` module to those in `constructs`
module.

Specific rules in the linter recognize this variable and modify their
expectations.

Motivation
The primary motivation is to move the code base towards [removal of the
construct compat layer][compat-rfc] as part of [CDKv2].

A large number of code changes to adopt "constructs" module can already
be done as part of CDKv1 without incurring breaking changes to the API.

This change enables these changes to be performed module-by-module. As
modules are migrated, this flag will be enabled, to ensure no
regression.

[CDKv2]: https://github.com/aws/aws-cdk-rfcs/blob/master/text/0079-cdk-2.0.md
[compat-rfc]: https://github.com/aws/aws-cdk-rfcs/blob/master/text/0192-remove-constructs-compat.md

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
Niranjan Jayakar authored Sep 23, 2020
1 parent e8e350b commit c179699
Show file tree
Hide file tree
Showing 18 changed files with 110 additions and 40 deletions.
3 changes: 2 additions & 1 deletion packages/@aws-cdk/aws-s3/lib/bucket-policy.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { PolicyDocument } from '@aws-cdk/aws-iam';
import { Construct, RemovalPolicy, Resource } from '@aws-cdk/core';
import { RemovalPolicy, Resource } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { IBucket } from './bucket';
import { CfnBucketPolicy } from './s3.generated';

Expand Down
3 changes: 2 additions & 1 deletion packages/@aws-cdk/aws-s3/lib/bucket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ import { EOL } from 'os';
import * as events from '@aws-cdk/aws-events';
import * as iam from '@aws-cdk/aws-iam';
import * as kms from '@aws-cdk/aws-kms';
import { Construct, Fn, IResource, Lazy, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/core';
import { Fn, IResource, Lazy, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { BucketPolicy } from './bucket-policy';
import { IBucketNotificationDestination } from './destination';
import { BucketNotifications } from './notifications-resource';
Expand Down
5 changes: 3 additions & 2 deletions packages/@aws-cdk/aws-s3/lib/util.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import * as cdk from '@aws-cdk/core';
import { IConstruct } from 'constructs';
import { BucketAttributes } from './bucket';

export function parseBucketArn(construct: cdk.IConstruct, props: BucketAttributes): string {
export function parseBucketArn(construct: IConstruct, props: BucketAttributes): string {

// if we have an explicit bucket ARN, use it.
if (props.bucketArn) {
Expand All @@ -22,7 +23,7 @@ export function parseBucketArn(construct: cdk.IConstruct, props: BucketAttribute
throw new Error('Cannot determine bucket ARN. At least `bucketArn` or `bucketName` is needed');
}

export function parseBucketName(construct: cdk.IConstruct, props: BucketAttributes): string | undefined {
export function parseBucketName(construct: IConstruct, props: BucketAttributes): string | undefined {

// if we have an explicit bucket name, use it.
if (props.bucketName) {
Expand Down
5 changes: 4 additions & 1 deletion packages/@aws-cdk/aws-s3/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@
"compat": "cdk-compat"
},
"cdk-build": {
"cloudformation": "AWS::S3"
"cloudformation": "AWS::S3",
"env": {
"AWSLINT_BASE_CONSTRUCT": "true"
}
},
"keywords": [
"aws",
Expand Down
3 changes: 2 additions & 1 deletion packages/@aws-cdk/aws-s3/test/test.aspect.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// import { expect, haveResource, haveResourceLike, SynthUtils } from '@aws-cdk/assert';
import { SynthUtils } from '@aws-cdk/assert';
import * as cdk from '@aws-cdk/core';
import { IConstruct } from 'constructs';
import { Test } from 'nodeunit';
import * as s3 from '../lib';

Expand Down Expand Up @@ -40,7 +41,7 @@ export = {
};

class BucketVersioningChecker implements cdk.IAspect {
public visit(node: cdk.IConstruct): void {
public visit(node: IConstruct): void {
if (node instanceof s3.CfnBucket) {
if (!node.versioningConfiguration ||
(!cdk.Tokenization.isResolvable(node.versioningConfiguration) && node.versioningConfiguration.status !== 'Enabled')) {
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/core/lib/construct-compat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export class Construct extends constructs.Construct implements IConstruct {
*/
public readonly node: ConstructNode;

constructor(scope: Construct, id: string) {
constructor(scope: constructs.Construct, id: string) {
super(scope, id, {
nodeFactory: {
createNode: (h: constructs.Construct, s: constructs.IConstruct, i: string) =>
Expand Down
7 changes: 4 additions & 3 deletions packages/@aws-cdk/core/lib/private/physical-name-generator.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as crypto from 'crypto';
import { Node } from 'constructs';
import { IResolvable, IResolveContext } from '../resolvable';
import { IResource } from '../resource';
import { Stack } from '../stack';
Expand All @@ -8,16 +9,16 @@ import { TokenMap } from './token-map';
export function generatePhysicalName(resource: IResource): string {
const stack = Stack.of(resource);
const stackPart = new PrefixNamePart(stack.stackName, 25);
const idPart = new SuffixNamePart(resource.node.uniqueId, 24);
const idPart = new SuffixNamePart(Node.of(resource).uniqueId, 24);

const region: string = stack.region;
if (Token.isUnresolved(region) || !region) {
throw new Error(`Cannot generate a physical name for ${resource.node.path}, because the region is un-resolved or missing`);
throw new Error(`Cannot generate a physical name for ${Node.of(resource).path}, because the region is un-resolved or missing`);
}

const account: string = stack.account;
if (Token.isUnresolved(account) || !account) {
throw new Error(`Cannot generate a physical name for ${resource.node.path}, because the account is un-resolved or missing`);
throw new Error(`Cannot generate a physical name for ${Node.of(resource).path}, because the account is un-resolved or missing`);
}

const parts = [stackPart, idPart]
Expand Down
5 changes: 3 additions & 2 deletions packages/@aws-cdk/core/lib/resource.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Construct } from 'constructs';
import { ArnComponents } from './arn';
import { Construct, IConstruct } from './construct-compat';
import { IConstruct, Construct as CoreConstruct } from './construct-compat';
import { Lazy } from './lazy';
import { generatePhysicalName, isGeneratedWhenNeededMarker } from './private/physical-name-generator';
import { IResolveContext } from './resolvable';
Expand Down Expand Up @@ -86,7 +87,7 @@ export interface ResourceProps {
/**
* A construct which represents an AWS resource.
*/
export abstract class Resource extends Construct implements IResource {
export abstract class Resource extends CoreConstruct implements IResource {
public readonly stack: Stack;
public readonly env: ResourceEnvironment;

Expand Down
16 changes: 9 additions & 7 deletions packages/@aws-cdk/core/lib/stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as fs from 'fs';
import * as path from 'path';
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import * as cxapi from '@aws-cdk/cx-api';
import { IConstruct, Node } from 'constructs';
import { Annotations } from './annotations';
import { App } from './app';
import { Arn, ArnComponents } from './arn';
Expand All @@ -10,7 +11,7 @@ import { CfnElement } from './cfn-element';
import { Fn } from './cfn-fn';
import { Aws, ScopedAws } from './cfn-pseudo';
import { CfnResource, TagType } from './cfn-resource';
import { Construct, IConstruct, ISynthesisSession } from './construct-compat';
import { Construct, ISynthesisSession } from './construct-compat';
import { ContextProvider } from './context-provider';
import { Environment } from './environment';
import { FeatureFlags } from './feature-flags';
Expand Down Expand Up @@ -169,11 +170,12 @@ export class Stack extends Construct implements ITaggable {
return c;
}

if (Stage.isStage(c) || !c.node.scope) {
throw new Error(`${construct.constructor?.name ?? 'Construct'} at '${construct.node.path}' should be created in the scope of a Stack, but no Stack found`);
const _scope = Node.of(c).scope;
if (Stage.isStage(c) || !_scope) {
throw new Error(`${construct.constructor?.name ?? 'Construct'} at '${Node.of(construct).path}' should be created in the scope of a Stack, but no Stack found`);
}

return _lookup(c.node.scope);
return _lookup(_scope);
}
}

Expand Down Expand Up @@ -934,7 +936,7 @@ export class Stack extends Construct implements ITaggable {
*/
private generateStackId(container: IConstruct | undefined) {
const rootPath = rootPathTo(this, container);
const ids = rootPath.map(c => c.node.id);
const ids = rootPath.map(c => Node.of(c).id);

// In unit tests our Stack (which is the only component) may not have an
// id, so in that case just pretend it's "Stack".
Expand Down Expand Up @@ -1051,7 +1053,7 @@ function cfnElements(node: IConstruct, into: CfnElement[] = []): CfnElement[] {
into.push(node);
}

for (const child of node.node.children) {
for (const child of Node.of(node).children) {
// Don't recurse into a substack
if (Stack.isStack(child)) { continue; }

Expand All @@ -1067,7 +1069,7 @@ function cfnElements(node: IConstruct, into: CfnElement[] = []): CfnElement[] {
* If no ancestor is given or the ancestor is not found, return the entire root path.
*/
export function rootPathTo(construct: IConstruct, ancestor?: IConstruct): IConstruct[] {
const scopes = construct.node.scopes;
const scopes = Node.of(construct).scopes;
for (let i = scopes.length - 2; i >= 0; i--) {
if (scopes[i] === ancestor) {
return scopes.slice(i + 1);
Expand Down
5 changes: 3 additions & 2 deletions packages/@aws-cdk/core/lib/stage.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as cxapi from '@aws-cdk/cx-api';
import { Construct, IConstruct } from './construct-compat';
import { IConstruct, Node } from 'constructs';
import { Construct } from './construct-compat';
import { Environment } from './environment';
import { collectRuntimeInformation } from './private/runtime-info';
import { synthesize } from './private/synthesis';
Expand Down Expand Up @@ -73,7 +74,7 @@ export class Stage extends Construct {
* @experimental
*/
public static of(construct: IConstruct): Stage | undefined {
return construct.node.scopes.reverse().slice(1).find(Stage.isStage);
return Node.of(construct).scopes.reverse().slice(1).find(Stage.isStage);
}

/**
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"exclude": [
"props-physical-name:@aws-cdk/aws-cloudformation.CustomResourceProps",
"construct-ctor:@aws-cdk/core.App.<initializer>",
"construct-ctor:@aws-cdk/core.Construct.<initializer>.params[0]",
"props-no-cfn-types:@aws-cdk/core.CfnOutputProps.condition",
"duration-prop-type:@aws-cdk/core.ResourceSignal.timeout",
"props-no-any:@aws-cdk/core.CfnParameterProps.default",
Expand Down
20 changes: 18 additions & 2 deletions packages/awslint/lib/rules/construct.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ export class ConstructReflection {
return typeRef.fqn;
}

/**
* @deprecated - use `CoreTypes.constructClass()` or `CoreTypes.baseConstructClass()` as appropriate
*/
public readonly ROOT_CLASS: reflect.ClassType; // cdk.Construct

public readonly fqn: string;
Expand Down Expand Up @@ -79,7 +82,7 @@ export class ConstructReflection {

constructLinter.add({
code: 'construct-ctor',
message: 'signature of all construct constructors should be "scope, id, props"',
message: 'signature of all construct constructors should be "scope, id, props". ' + baseConstructAddendum(),
eval: e => {
// only applies to non abstract classes
if (e.ctx.classType.abstract) {
Expand All @@ -93,9 +96,15 @@ constructLinter.add({

const expectedParams = new Array<MethodSignatureParameterExpectation>();

let baseType;
if (process.env.AWSLINT_BASE_CONSTRUCT && !initializer.parentType.name.startsWith('Cfn')) {
baseType = e.ctx.core.baseConstructClass;
} else {
baseType = e.ctx.core.constructClass;
}
expectedParams.push({
name: 'scope',
type: e.ctx.core.constructClass.fqn,
type: baseType.fqn,
});

expectedParams.push({
Expand Down Expand Up @@ -276,3 +285,10 @@ constructLinter.add({
}
},
});

function baseConstructAddendum(): string {
if (!process.env.AWSLINT_BASE_CONSTRUCT) {
return 'If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run';
}
return '';
}
28 changes: 25 additions & 3 deletions packages/awslint/lib/rules/core-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,18 @@ import { getDocTag } from './util';
const CORE_MODULE = '@aws-cdk/core';
enum CoreTypesFqn {
CfnResource = '@aws-cdk/core.CfnResource',
Construct = '@aws-cdk/core.Construct',
ConstructInterface = '@aws-cdk/core.IConstruct',
Resource = '@aws-cdk/core.Resource',
ResourceInterface = '@aws-cdk/core.IResource',
ResolvableInterface = '@aws-cdk/core.IResolvable',
PhysicalName = '@aws-cdk/core.PhysicalName'
PhysicalName = '@aws-cdk/core.PhysicalName',

BaseConstruct = 'constructs.Construct',
BaseConstructInterface = 'constructs.Construct',

/** @deprecated - use BaseConstruct */
Construct = '@aws-cdk/core.Construct',
/** @deprecated - use BaseConstructInterface */
ConstructInterface = '@aws-cdk/core.IConstruct',
}

export class CoreTypes {
Expand Down Expand Up @@ -86,18 +92,34 @@ export class CoreTypes {

/**
* @returns `classType` for the core type Construct
* @deprecated - use `baseConstructClass()`
*/
public get constructClass() {
return this.sys.findClass(CoreTypesFqn.Construct);
}

/**
* @returns `classType` for the core type Construct
*/
public get baseConstructClass() {
return this.sys.findClass(CoreTypesFqn.BaseConstruct);
}

/**
* @returns `interfacetype` for the core type Construct
* @deprecated - use `baseConstructInterface()`
*/
public get constructInterface() {
return this.sys.findInterface(CoreTypesFqn.ConstructInterface);
}

/**
* @returns `interfacetype` for the core type Construct
*/
public get baseConstructInterface() {
return this.sys.findInterface(CoreTypesFqn.BaseConstructInterface);
}

/**
* @returns `classType` for the core type Construct
*/
Expand Down
19 changes: 15 additions & 4 deletions packages/awslint/lib/rules/imports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,18 @@ importsLinter.add({

importsLinter.add({
code: 'from-signature',
message: 'invalid method signature for fromXxx method',
message: 'invalid method signature for fromXxx method. ' + baseConstructAddendum(),
eval: e => {
for (const method of e.ctx.fromMethods) {

// "fromRoleArn" => "roleArn"
const argName = e.ctx.resource.basename[0].toLocaleLowerCase() + method.name.slice('from'.length + 1);

const baseType = process.env.AWSLINT_BASE_CONSTRUCT ? e.ctx.resource.core.baseConstructClass :
e.ctx.resource.core.constructClass;
e.assertSignature(method, {
parameters: [
{ name: 'scope', type: e.ctx.resource.construct.ROOT_CLASS },
{ name: 'scope', type: baseType },
{ name: 'id', type: 'string' },
{ name: argName, type: 'string' },
],
Expand All @@ -84,15 +86,17 @@ importsLinter.add({

importsLinter.add({
code: 'from-attributes',
message: 'static fromXxxAttributes is a factory of IXxx from its primitive attributes',
message: 'static fromXxxAttributes is a factory of IXxx from its primitive attributes. ' + baseConstructAddendum(),
eval: e => {
if (!e.ctx.fromAttributesMethod) {
return;
}

const baseType = process.env.AWSLINT_BASE_CONSTRUCT ? e.ctx.resource.core.baseConstructClass
: e.ctx.resource.core.constructClass;
e.assertSignature(e.ctx.fromAttributesMethod, {
parameters: [
{ name: 'scope', type: e.ctx.resource.construct.ROOT_CLASS },
{ name: 'scope', type: baseType },
{ name: 'id', type: 'string' },
{ name: 'attrs', type: e.ctx.attributesStruct },
],
Expand All @@ -111,3 +115,10 @@ importsLinter.add({
e.assert(e.ctx.attributesStruct, e.ctx.attributesStructName);
},
});

function baseConstructAddendum(): string {
if (!process.env.AWSLINT_BASE_CONSTRUCT) {
return 'If the construct is using the "constructs" module, set the environment variable "AWSLINT_BASE_CONSTRUCT" and re-run';
}
return '';
}
9 changes: 5 additions & 4 deletions tools/cdk-build-tools/bin/cdk-build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,10 @@ async function main() {
.argv;

const options = cdkBuildOptions();
const env = options.env;

if (options.pre) {
await shell(options.pre, { timers });
await shell(options.pre, { timers, env });
}

// See if we need to call cfn2ts
Expand All @@ -38,15 +39,15 @@ async function main() {
// There can be multiple scopes, ensuring it's always an array.
options.cloudformation = [options.cloudformation];
}
await shell(['cfn2ts', ...options.cloudformation.map(scope => `--scope=${scope}`)], { timers });
await shell(['cfn2ts', ...options.cloudformation.map(scope => `--scope=${scope}`)], { timers, env });
}

const overrides: CompilerOverrides = { eslint: args.eslint, jsii: args.jsii, tsc: args.tsc };
await compileCurrentPackage(timers, overrides);
await compileCurrentPackage(options, timers, overrides);
await lintCurrentPackage(options, overrides);

if (options.post) {
await shell(options.post, { timers });
await shell(options.post, { timers, env });
}
}

Expand Down
Loading

0 comments on commit c179699

Please sign in to comment.