-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Framework: Tokens can be converted to strings #518
Changes from 10 commits
3796389
9469a71
e47741b
f6d01a8
6e79bb8
a72d14d
42becf3
1b60e45
d1fc358
58602a9
74f4fe7
b15684d
5753e31
4206b85
85cfa81
a3ec610
7361815
a7b41e2
6aa9b77
c5b3b68
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import { DEFAULT_ENGINE_NAME, ProvisioningEngine, StringFragment } from "../core/engine-strings"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole “provisioning engine framework” feels like it’s future proofing for something we really don’t know how to design yet. May I suggest that the token plugin system will be kept within the confines of the “tokens framework” and use domain-specific semantics. For example, if the CDK will be used to synthesize pure Amazon State Language documents, then there is no provisioning engine involved, but there still might be tokenization heuristics. Quite possibly, we might want to extract the tokens framework into a separate general-purpose and independent npm module, unrelated to cloud resource provisioning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well I did have it all in one file first, but it became quite large and mixed too many different concerns for my tastes. I just need some place to put this logic which is preferably a separate file, so it is "string handling for engines in an IoC fashion", which is this.
Regardless of the language that is being stringified (ASL or otherwise), if there is Token stringification to be done it will involve stitching string literals and intrinsics together. This must be done with an engine-specific If you use Amazon State Language on Terraform, we need Granted, maybe the filename is not fantastic here, and maybe the interface could be replaced with a single function, but that's about all the undesigning leeway we have. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not referring to the case where some other provisioning engine is used, I am referring to the case where the CDK is used to just synthesize other types of documents. I agree, tokenization/concat will still need to happen in those cases, but the provider will not be a "provisioning engine" but some other document format. That's why I suggested to bind this terminology to the resolve/tokens domain and not to the resource provisioning domain. Then perhaps some of the abstractions will be an overkill. For example |
||
import { IntrinsicToken } from "../core/tokens"; | ||
|
||
/** | ||
* The default intrinsics Token engine for CloudFormation | ||
*/ | ||
export const CLOUDFORMATION_ENGINE = 'cloudformation'; | ||
|
||
/** | ||
* Base class for CloudFormation built-ins | ||
*/ | ||
export class CloudFormationIntrinsicToken extends IntrinsicToken { | ||
protected readonly engine: string = CLOUDFORMATION_ENGINE; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of using a string value for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not a But I guess I'm with ya on them having a direct reference to the engine, instead of indirected through a string pointing to a table. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
|
||
import { FnConcat } from "./fn"; | ||
|
||
const cloudFormationEngine = { | ||
/** | ||
* In CloudFormation, we combine strings by wrapping them in FnConcat | ||
*/ | ||
combineStringFragments(fragments: StringFragment[]) { | ||
return new FnConcat(...fragments.map(f => f.value)); | ||
} | ||
}; | ||
|
||
ProvisioningEngine.register(CLOUDFORMATION_ENGINE, cloudFormationEngine); | ||
ProvisioningEngine.register(DEFAULT_ENGINE_NAME, cloudFormationEngine); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need the default? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We used to at some point because we could at some point stitch values together where none of them would be an intrinsic (and so it would not be obvious which engine to use) but I guess at this point that's something I can do in the framework. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,8 @@ | ||
import { Token } from '../core/tokens'; | ||
import { CloudFormationIntrinsicToken } from './intrinsics'; | ||
|
||
export class PseudoParameter extends Token { | ||
export class PseudoParameter extends CloudFormationIntrinsicToken { | ||
constructor(name: string) { | ||
super(() => ({ Ref: name })); | ||
super(() => ({ Ref: name }), name); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this need to be lazy? We can plug in a concrete value |
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ import { Construct } from '../core/construct'; | |
import { Token } from '../core/tokens'; | ||
import { capitalizePropertyNames, ignoreEmpty } from '../core/util'; | ||
import { Condition } from './condition'; | ||
import { CloudFormationIntrinsicToken } from './intrinsics'; | ||
import { CreationPolicy, DeletionPolicy, UpdatePolicy } from './resource-policy'; | ||
import { IDependable, Referenceable, StackElement } from './stack'; | ||
|
||
|
@@ -82,7 +83,7 @@ export class Resource extends Referenceable { | |
* @param attributeName The name of the attribute. | ||
*/ | ||
public getAtt(attributeName: string): Token { | ||
return new Token(() => ({ 'Fn::GetAtt': [this.logicalId, attributeName] })); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn’t need to be lazy |
||
return new CloudFormationIntrinsicToken(() => ({ 'Fn::GetAtt': [this.logicalId, attributeName] }), `${this.logicalId}.${attributeName}`); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ import { App } from '../app'; | |
import { Construct, PATH_SEP } from '../core/construct'; | ||
import { resolve, Token } from '../core/tokens'; | ||
import { Environment } from '../environment'; | ||
import { CloudFormationIntrinsicToken } from './intrinsics'; | ||
import { HashedAddressingScheme, IAddressingScheme, LogicalIDs } from './logical-id'; | ||
import { Resource } from './resource'; | ||
|
||
|
@@ -391,8 +392,8 @@ export abstract class Referenceable extends StackElement { | |
/** | ||
* Returns a token to a CloudFormation { Ref } that references this entity based on it's logical ID. | ||
*/ | ||
public get ref() { | ||
return new Token(() => ({ Ref: this.logicalId })); | ||
public get ref(): Token { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should just define a ‘Ref’ class for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for that. It doesn't add a lot of value and no user is ever going to instantiate it. |
||
return new CloudFormationIntrinsicToken(() => ({ Ref: this.logicalId }), `${this.logicalId}.Ref`); | ||
} | ||
} | ||
|
||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/** | ||
* Mark a given object as a provisioning engine-intrinsic value. | ||
* | ||
* Any value that has been marked as intrinsic to a provisioning engine | ||
* will escape all further type checks and attempts to manipulate, and be | ||
* passed on as-is to the final provisioning engine. | ||
* | ||
* Note that this is separate from a Token: a Token represents a lazy value. | ||
* The result of evaluating a Token is a value, which may or may not be an | ||
* engine intrinsic value. If you want to combine the two, see `IntrinsicToken`. | ||
*/ | ||
export function markAsIntrinsic(x: any, engine: string): any { | ||
// This could have been a wrapper class, but that breaks all test.deepEqual()s. | ||
// So instead, it's implemented as a hidden property on the object (which is | ||
// hidden from JSON.stringify() and test.deepEqual(). | ||
|
||
Object.defineProperty(x, INTRINSIC_VALUE_PROPERTY, { | ||
value: engine, | ||
enumerable: false, | ||
writable: false, | ||
}); | ||
return x; | ||
} | ||
|
||
/** | ||
* Return whether the given value is an intrinsic | ||
*/ | ||
export function isIntrinsic(x: any): boolean { | ||
return x[INTRINSIC_VALUE_PROPERTY] !== undefined; | ||
} | ||
|
||
/** | ||
* Return the intrinsic engine for the given intrinsic value | ||
*/ | ||
export function intrinsicEngine(x: any): string | undefined { | ||
return x[INTRINSIC_VALUE_PROPERTY]; | ||
} | ||
|
||
/** | ||
* The hidden marker property that marks an object as an engine-intrinsic value. | ||
*/ | ||
const INTRINSIC_VALUE_PROPERTY = '__intrinsicValue__'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wraps a token and
CloudFormationJSON.stringify
also returns aToken
. Can we eliminate one layer?