Skip to content

Commit

Permalink
Stop generating "| Token" in L1 properties that accept "string" or "s…
Browse files Browse the repository at this point in the history
…tring[]"
  • Loading branch information
Elad Ben-Israel committed Jan 7, 2019
1 parent db92ac0 commit 62f35be
Show file tree
Hide file tree
Showing 8 changed files with 38 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ export class AutoScalingGroup extends cdk.Construct implements IAutoScalingGroup

// use delayed evaluation
const machineImage = props.machineImage.getImage(this);
const userDataToken = new cdk.Token(() => cdk.Fn.base64((machineImage.os.createUserData(this.userDataLines))));
const userDataToken = new cdk.Token(() => cdk.Fn.base64((machineImage.os.createUserData(this.userDataLines)))).toString();

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Jan 7, 2019

Contributor

Should we make that helper function that takes a function and returns the stringified version of it?

export class Lazy {
  public static stringValue(fn: any) {
    return new cdk.Token(fn).toString();
  }
}

const userDataToken = Lazy.stringValue(() => ...);

?

This comment has been minimized.

Copy link
@eladb

eladb Jan 7, 2019

Contributor

Oh yes, please!

I think we should deprecate cdk.Token altogether, but I am waiting for #1436 before that

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Jan 7, 2019

Contributor

Okay.

const securityGroupsToken = new cdk.Token(() => this.securityGroups.map(sg => sg.securityGroupId));

const launchConfig = new CfnLaunchConfiguration(this, 'LaunchConfig', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export class GitHubSourceAction extends actions.SourceAction {
new CfnWebhook(this, 'WebhookResource', {
authentication: 'GITHUB_HMAC',
authenticationConfiguration: {
secretToken: props.oauthToken,
secretToken: props.oauthToken.toString(),

This comment has been minimized.

Copy link
@rix0rrr

rix0rrr Jan 7, 2019

Contributor

Seems like this should have been a retyping of the oauthToken argument, no?

This comment has been minimized.

Copy link
@eladb

eladb Jan 7, 2019

Contributor

It's currently Secret, so I think we should keep it as is for now (and maybe think how to remodel this class)

},
filters: [
{
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ecs/lib/base/task-definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ export class TaskDefinition extends cdk.Construct {
const taskDef = new CfnTaskDefinition(this, 'Resource', {
containerDefinitions: new cdk.Token(() => this.containers.map(x => x.renderContainerDefinition())),
volumes: new cdk.Token(() => this.volumes),
executionRoleArn: new cdk.Token(() => this.executionRole && this.executionRole.roleArn),
executionRoleArn: new cdk.Token(() => this.executionRole && this.executionRole.roleArn).toString(),
family: this.family,
taskRoleArn: this.taskRole.roleArn,
requiresCompatibilities: [
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-iam/lib/policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export class Policy extends Construct implements IDependable {

const resource = new CfnPolicy(this, 'Resource', {
policyDocument: this.document,
policyName: new Token(() => this.policyName),
policyName: new Token(() => this.policyName).toString(),
roles: undefinedIfEmpty(() => this.roles.map(r => r.roleName)),
users: undefinedIfEmpty(() => this.users.map(u => u.userName)),
groups: undefinedIfEmpty(() => this.groups.map(g => g.groupName)),
Expand Down
8 changes: 4 additions & 4 deletions packages/@aws-cdk/aws-logs/lib/cross-account-destination.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,12 @@ export class CrossAccountDestination extends cdk.Construct implements ILogSubscr
super(scope, id);

// In the underlying model, the name is not optional, but we make it so anyway.
const destinationName = props.destinationName || new cdk.Token(() => this.generateUniqueName());
const destinationName = props.destinationName || new cdk.Token(() => this.generateUniqueName()).toString();

this.resource = new CfnDestination(this, 'Resource', {
destinationName,
// Must be stringified policy
destinationPolicy: new cdk.Token(() => this.stringifiedPolicyDocument()),
destinationPolicy: this.lazyStringifiedPolicyDocument(),
roleArn: props.role.roleArn,
targetArn: props.targetArn
});
Expand Down Expand Up @@ -94,7 +94,7 @@ export class CrossAccountDestination extends cdk.Construct implements ILogSubscr
/**
* Return a stringified JSON version of the PolicyDocument
*/
private stringifiedPolicyDocument() {
return this.policyDocument.isEmpty ? '' : cdk.CloudFormationJSON.stringify(cdk.resolve(this.policyDocument));
private lazyStringifiedPolicyDocument() {
return new cdk.Token(() => this.policyDocument.isEmpty ? '' : cdk.CloudFormationJSON.stringify(cdk.resolve(this.policyDocument))).toString();
}
}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-route53/lib/hosted-zone.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ export class PrivateHostedZone extends HostedZone {
}

function toVpcProperty(vpc: ec2.IVpcNetwork): CfnHostedZone.VPCProperty {
return { vpcId: vpc.vpcId, vpcRegion: new cdk.AwsRegion() };
return { vpcId: vpc.vpcId, vpcRegion: new cdk.AwsRegion().toString() };
}

function determineHostedZoneProps(props: PublicHostedZoneProps) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export class CloudFormationJSON {
* All Tokens substituted in this way must return strings, or the evaluation
* in CloudFormation will fail.
*/
public static stringify(obj: any): Token {
public static stringify(obj: any): string {
return new Token(() => {
// Resolve inner value first so that if they evaluate to literals, we
// maintain the type (and discard 'undefined's).
Expand All @@ -31,7 +31,7 @@ export class CloudFormationJSON {
// We can just directly return this value, since resolve() will be called
// on our return value anyway.
return JSON.stringify(deepReplaceIntrinsics(resolved));
});
}).toString();

/**
* Recurse into a structure, replace all intrinsics with IntrinsicTokens.
Expand Down
29 changes: 27 additions & 2 deletions tools/cfn2ts/lib/codegen.ts
Original file line number Diff line number Diff line change
Expand Up @@ -565,8 +565,14 @@ export default class CodeGenerator {
alternatives.push(this.renderTypeUnion(resourceContext, types));
}

// Always
alternatives.push(genspec.TOKEN_NAME.fqn);
// Only if this property is not of a "tokenizable type" (string, string[],
// number in the future) we add a type union for `cdk.Token`. We rather
// everything to be tokenizable because there are languages that do not
// support union types (i.e. Java, .NET), so we lose type safety if we have
// a union.
if (!tokenizableType(alternatives)) {
alternatives.push(genspec.TOKEN_NAME.fqn);
}

return alternatives.join(' | ');
}
Expand Down Expand Up @@ -618,3 +624,22 @@ function mapperNames(types: genspec.CodeName[]): string {
function quoteCode(code: string): string {
return '``' + code + '``';
}

function tokenizableType(alternatives: string[]) {
if (alternatives.length > 1) {
return false;
}

const type = alternatives[0];
if (type === 'string') {
return true;
}

if (type === 'string[]') {
return true;
}

// TODO: number

return false;
}

0 comments on commit 62f35be

Please sign in to comment.