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(context-providers) Enable arguments to be passed as property object #823

Merged
merged 13 commits into from
Oct 22, 2018
Merged
13 changes: 8 additions & 5 deletions packages/@aws-cdk/aws-ec2/lib/machine-image.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,11 @@ export class WindowsImage implements IMachineImageSource {
* Return the image to use in the given context
*/
public getImage(parent: Construct): MachineImage {
const ssmProvider = new SSMParameterProvider(parent);
const ssmProvider = new SSMParameterProvider(parent, {
parameterName: this.imageParameterName(this.version),
});

const parameterName = this.imageParameterName(this.version);
const ami = ssmProvider.getString(parameterName);
const ami = ssmProvider.parameterValue();
return new MachineImage(ami, new WindowsOS());
}

Expand Down Expand Up @@ -98,8 +99,10 @@ export class AmazonLinuxImage implements IMachineImageSource {

const parameterName = '/aws/service/ami-amazon-linux-latest/' + parts.join('-');

const ssmProvider = new SSMParameterProvider(parent);
const ami = ssmProvider.getString(parameterName);
const ssmProvider = new SSMParameterProvider(parent, {
parameterName,
});
const ami = ssmProvider.parameterValue();
return new MachineImage(ami, new LinuxOS());
}
}
Expand Down
167 changes: 103 additions & 64 deletions packages/@aws-cdk/cdk/lib/context.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import cxapi = require('@aws-cdk/cx-api');
import { Stack } from './cloudformation/stack';
import { Construct } from './core/construct';

const AVAILABILITY_ZONES_PROVIDER = 'availability-zones';
const SSM_PARAMETER_PROVIDER = 'ssm';
const HOSTED_ZONE_PROVIDER = 'hosted-zone';

/**
* Base class for the model side of context providers
Expand All @@ -17,38 +19,48 @@ export class ContextProvider {

private readonly stack: Stack;

constructor(private context: Construct) {
constructor(
private readonly context: Construct,
private readonly provider: string,
private readonly props: {[key: string]: any} = {}) {
this.stack = Stack.find(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we just go like this in the constructor:

this.props.account = this.stack.env.account;
this.props.region = this.stack.env.region;

And then in all other places we just use this.props.

}

public get key(): string {
const propStrings: string[] = propsToArray({
...this.props,
...{account: this.account, region: this.region},
Copy link
Contributor

Choose a reason for hiding this comment

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

Literals don't need a splat. Could just be:

{
   ...this.props,
   account: this.account,
   region: this.region
}

});
return `${this.provider}:${propStrings.join(':')}`;
}
/**
* Read a provider value, verifying it's a string
* @param provider The name of the context provider
* @param scope The scope (e.g. account/region) for the value
* @param args Any arguments
* @param defaultValue The value to return if there is no value defined for this context key
*/
public getStringValue(
provider: string,
scope: undefined | string[],
args: string[],
defaultValue: string): string {
public getStringValue( defaultValue: string): string {
// if scope is undefined, this is probably a test mode, so we just
// return the default value
if (!scope) {
this.context.addError(formatMissingScopeError(provider, args));
if (!this.account || !this.region) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interestingly, this now means something else. Because previously there was a distinction between "not having a scope" and "not having an account/region", and now there isn't anymore.

This only comes into play if we have context providers that don't need both an account and a region, which we don't have right now anyway so I'm okay with this change and we can always fix it once we do. But it's something to consider.

this.context.addError(formatMissingScopeError(this.provider, this.props));
return defaultValue;
}
const key = colonQuote([provider].concat(scope).concat(args)).join(':');
const value = this.context.getContext(key);

const value = this.context.getContext(this.key);

if (value != null) {
if (typeof value !== 'string') {
throw new TypeError(`Expected context parameter '${key}' to be a string, but got '${value}'`);
throw new TypeError(`Expected context parameter '${this.key}' to be a string, but got '${value}'`);
}
return value;
}

this.stack.reportMissingContext(key, { provider, scope, args });
this.stack.reportMissingContext(this.key, {
provider: this.provider,
props: { ...this.props, ...{region: this.region, account: this.account} },
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid this duplication, I'm thinking it might even be worth adding a private getter for the "full props", or embed {account, region} into the object at creation time.

});
return defaultValue;
}

Expand All @@ -60,50 +72,38 @@ export class ContextProvider {
* @param defaultValue The value to return if there is no value defined for this context key
*/
public getStringListValue(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I can dry this function up by passing a function here. I don't think I can use generics here. I suppose I could extract this to a non exported function and pass everything into it, but I think just passing a validateType(value: any): boolean{} will probably do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually that is also blocked by JSII ... I'm not sure we can really DRY this up much then?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine as-is.

provider: string,
scope: undefined | string[],
args: string[],
defaultValue: string[]): string[] {
// if scope is undefined, this is probably a test mode, so we just
// return the default value and report an error so this in not accidentally used
// in the toolkit
if (!scope) {
// tslint:disable-next-line:max-line-length
this.context.addError(formatMissingScopeError(provider, args));
return defaultValue;
}
// if scope is undefined, this is probably a test mode, so we just
// return the default value and report an error so this in not accidentally used
// in the toolkit
if (!this.account || !this.region) {
this.context.addError(formatMissingScopeError(this.provider, this.props));
return defaultValue;
}

const key = colonQuote([provider].concat(scope).concat(args)).join(':');
const value = this.context.getContext(key);
const value = this.context.getContext(this.key);

if (value != null) {
if (!value.map) {
throw new Error(`Context value '${key}' is supposed to be a list, got '${value}'`);
if (value != null) {
if (!value.map) {
throw new Error(`Context value '${this.key}' is supposed to be a list, got '${value}'`);
}
return value;
}
return value;
}

this.stack.reportMissingContext(key, { provider, scope, args });
return defaultValue;
}
this.stack.reportMissingContext(this.key, {
provider: this.provider,
props: { ...this.props, ...{region: this.region, account: this.account} },
});

/**
* Helper function to wrap up account and region into a scope tuple
*/
public accountRegionScope(providerDescription: string): undefined | string[] {
const stack = Stack.find(this.context);
if (!stack) {
throw new Error(`${providerDescription}: construct must be in a stack`);
return defaultValue;
}

const account = stack.env.account;
const region = stack.env.region;

if (account == null || region == null) {
return undefined;
}
private get account(): string | undefined {
return this.stack.env.account;
}

return [account, region];
private get region(): string | undefined {
return this.stack.env.region;
}
}

Expand All @@ -113,8 +113,8 @@ export class ContextProvider {
* We'll use $ as a quoting character, for no particularly good reason other
* than that \ is going to lead to quoting hell when the keys are stored in JSON.
*/
function colonQuote(xs: string[]): string[] {
return xs.map(x => x.replace('$', '$$').replace(':', '$:'));
function colonQuote(xs: string): string {
return xs.replace('$', '$$').replace(':', '$:');
}

/**
Expand All @@ -124,46 +124,85 @@ export class AvailabilityZoneProvider {
private provider: ContextProvider;

constructor(context: Construct) {
this.provider = new ContextProvider(context);
this.provider = new ContextProvider(context, AVAILABILITY_ZONES_PROVIDER);
}

/**
* Return the list of AZs for the current account and region
*/
public get availabilityZones(): string[] {
return this.provider.getStringListValue(AVAILABILITY_ZONES_PROVIDER,
this.provider.accountRegionScope('AvailabilityZoneProvider'),
[],
['dummy1a', 'dummy1b', 'dummy1c']);

return this.provider.getStringListValue(['dummy1a', 'dummy1b', 'dummy1c']);
}
}

export interface SSMParameterProviderProps {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need docs here

parameterName: string;
}
/**
* Context provider that will read values from the SSM parameter store in the indicated account and region
*/
export class SSMParameterProvider {
private provider: ContextProvider;

constructor(context: Construct) {
this.provider = new ContextProvider(context);
constructor(context: Construct, props: SSMParameterProviderProps) {
this.provider = new ContextProvider(context, SSM_PARAMETER_PROVIDER, props);
}

/**
* Return the SSM parameter string with the indicated key
*/
public getString(parameterName: string): any {
const scope = this.provider.accountRegionScope('SSMParameterProvider');
return this.provider.getStringValue(SSM_PARAMETER_PROVIDER, scope, [parameterName], 'dummy');
public parameterValue(): any {
return this.provider.getStringValue('dummy');
}
}

function formatMissingScopeError(provider: string, args: string[]) {
let s = `Cannot determine scope for context provider ${provider}`;
if (args.length > 0) {
s += JSON.stringify(args);
/**
* Context provider that will lookup the Hosted Zone ID for the given arguments
*/
export class HostedZoneProvider {
private provider: ContextProvider;
constructor(context: Construct, props: cxapi.HostedZoneProviderProps) {
this.provider = new ContextProvider(context, HOSTED_ZONE_PROVIDER, props);
}
s += '.';
/**
* Return the hosted zone meeting the filter
*/
public zoneId(): string {
return this.provider.getStringValue('dummy-zone');
}
}

function formatMissingScopeError(provider: string, props: {[key: string]: string}) {
let s = `Cannot determine scope for context provider ${provider}`;
const propsString = Object.keys(props).map( key => (`${key}=${props[key]}`));
s += ` with props: ${propsString}.`;
s += '\n';
s += 'This usually happens when AWS credentials are not available and the default account/region cannot be determined.';
return s;
}

function propsToArray(props: {[key: string]: any}): string[] {
const propArray: string[] = [];
const keys = Object.keys(props);
keys.sort();
for (const key of keys) {
switch (typeof props[key]) {
case 'object': {
const childObjStrs = propsToArray(props[key]);
const qualifiedChildStr = childObjStrs.map( child => (`${key}.${child}`)).join(':');
propArray.push(qualifiedChildStr);
break;
}
case 'string': {
propArray.push(`${key}=${colonQuote(props[key])}`);
break;
}
default: {
propArray.push(`${key}=${JSON.stringify(props[key])}`);
break;
}
}
}
return propArray;
}
68 changes: 34 additions & 34 deletions packages/@aws-cdk/cdk/test/test.app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,16 +254,23 @@ export = {
super(parent, name, props);

this.reportMissingContext('missing-context-key', {
provider: 'ctx-provider',
args: [ 'arg1', 'arg2' ],
scope: [ 'scope1', 'scope2' ]
});
provider: 'fake',
props: {
account: '12345689012',
region: 'ab-north-1',
},
},
);

this.reportMissingContext('missing-context-key-2', {
provider: 'ctx-provider',
args: [ 'arg1', 'arg2' ],
scope: [ 'scope1', 'scope2' ]
});
provider: 'fake2',
props: {
foo: 'bar',
account: '12345689012',
region: 'ab-south-1',
},
},
);
}
}

Expand All @@ -275,27 +282,20 @@ export = {

test.deepEqual(response.stacks[0].missing, {
"missing-context-key": {
provider: "ctx-provider",
args: [
"arg1",
"arg2"
],
scope: [
"scope1",
"scope2"
]
provider: 'fake',
props: {
account: '12345689012',
region: 'ab-north-1',
},
},
"missing-context-key-2": {
provider: "ctx-provider",
args: [
"arg1",
"arg2"
],
scope: [
"scope1",
"scope2"
]
}
provider: 'fake2',
props: {
account: '12345689012',
region: 'ab-south-1',
foo: 'bar',
},
},
});

test.done();
Expand All @@ -309,15 +309,15 @@ export = {
test.deepEqual(resp, {
stacks: [
{
name: "stack1",
environment: {
name: "12345/us-east-1",
account: "12345",
region: "us-east-1"
}
name: "stack1",
environment: {
name: "12345/us-east-1",
account: "12345",
region: "us-east-1"
}
},
{
name: "stack2"
name: "stack2"
}
]
});
Expand Down
Loading