Skip to content

Add type configuration support to the typescript cli plugin #67

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

Merged
merged 11 commits into from
Jul 27, 2022
Merged
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -139,3 +139,5 @@ tests/**/*.d.ts
# Node.js
node_modules/
coverage/

*.iml
4 changes: 2 additions & 2 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ repos:
- repo: https://github.com/ambv/black
rev: stable
hooks:
- id: black
# language_version: python3.6
- id: black
Copy link
Contributor

Choose a reason for hiding this comment

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

was this indent accidental? if so, is it possible to remove this file from the commit?

Choose a reason for hiding this comment

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

probably an IDE did that automatically or a linter wanted it. yaml doesn't care and conventions vary whether list items should be indented two spaces or not. i note all the other blocks in this file do indent list items so this change makes it consistent. i'd recommend keeping but happy to remove if you prefer.

# language_version: python3.6
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v2.0.0
hooks:
Expand Down
19 changes: 16 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ If you are using this package to build resource providers for CloudFormation, in

**Prerequisites**

- Python version 3.6 or above
- [SAM CLI](https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/serverless-sam-cli-install.html)
- Your choice of TypeScript IDE
- Python version 3.6 or above
- [SAM CLI](https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/serverless-sam-cli-install.html)
- Your choice of TypeScript IDE

**Installation**

Expand Down Expand Up @@ -75,6 +75,19 @@ pip3 install \

That ensures neither is accidentally installed from PyPI.

For changes to the typescript library "@amazon-web-services-cloudformation/cloudformation-cli-typescript-lib" pack up the compiled javascript:

```shell
npm run build
npm pack
```

You can then install this in a cfn resource project using:

```shell
npm install ../path/to/cloudformation-cli-typescript-plugin/amazon-web-services-cloudformation-cloudformation-cli-typescript-lib-1.0.1.tgz
```

Linting and running unit tests is done via [pre-commit](https://pre-commit.com/), and so is performed automatically on commit after being installed (`pre-commit install`). The continuous integration also runs these checks. Manual options are available so you don't have to commit:

```shell
Expand Down
13 changes: 13 additions & 0 deletions python/rpdk/typescript/codegen.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,13 +161,24 @@ def generate(self, project):

models = resolve_models(project.schema)

if project.configuration_schema:
configuration_models = resolve_models(
project.configuration_schema, "TypeConfigurationModel"
)
Comment on lines +165 to +167
Copy link
Contributor

Choose a reason for hiding this comment

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

would be great to achieve parity with other plugins:

configuration_schema_path =project.root / project.configuration_schema_filename
project.write_configuration_schema(configuration_schema_path)

Copy link

Choose a reason for hiding this comment

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

@ammokhov Not sure what this does. As far as I know, we want all the models, TypeConfiguration included to be generated within models.ts. Am I missunderstanding something?

else:
configuration_models = {"TypeConfigurationModel": {}}

models.update(configuration_models)

path = self.package_root / "models.ts"
LOG.debug("Writing file: %s", path)
template = self.env.get_template("models.ts")

contents = template.render(
lib_name=SUPPORT_LIB_NAME,
type_name=project.type_name,
models=models,
contains_type_configuration=project.configuration_schema,
primaryIdentifier=project.schema.get("primaryIdentifier", []),
additionalIdentifiers=project.schema.get("additionalIdentifiers", []),
)
Expand All @@ -176,6 +187,8 @@ def generate(self, project):
LOG.debug("Generate complete")

def _pre_package(self, build_path):
# Caller should own/delete this, not us.
# pylint: disable=consider-using-with
f = TemporaryFile("w+b")

# pylint: disable=unexpected-keyword-arg
Expand Down
30 changes: 23 additions & 7 deletions python/rpdk/typescript/templates/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
ResourceHandlerRequest,
SessionProxy,
} from '{{lib_name}}';
import { ResourceModel } from './models';
import { ResourceModel, TypeConfigurationModel } from './models';

interface CallbackContext extends Record<string, any> {}

Expand All @@ -25,14 +25,17 @@ class Resource extends BaseResource<ResourceModel> {
* @param request The request object for the provisioning request passed to the implementor
* @param callbackContext Custom context object to allow the passing through of additional
* state or metadata between subsequent retries
* @param typeConfiguration Configuration data for this resource type, in the given account
* and region
* @param logger Logger to proxy requests to default publishers
*/
@handlerEvent(Action.Create)
public async create(
session: Optional<SessionProxy>,
request: ResourceHandlerRequest<ResourceModel>,
callbackContext: CallbackContext,
logger: LoggerProxy
logger: LoggerProxy,
typeConfiguration: TypeConfigurationModel,
): Promise<ProgressEvent<ResourceModel, CallbackContext>> {
const model = new ResourceModel(request.desiredResourceState);
const progress = ProgressEvent.progress<ProgressEvent<ResourceModel, CallbackContext>>(model);
Expand Down Expand Up @@ -63,14 +66,17 @@ class Resource extends BaseResource<ResourceModel> {
* @param request The request object for the provisioning request passed to the implementor
* @param callbackContext Custom context object to allow the passing through of additional
* state or metadata between subsequent retries
* @param typeConfiguration Configuration data for this resource type, in the given account
* and region
* @param logger Logger to proxy requests to default publishers
*/
@handlerEvent(Action.Update)
public async update(
session: Optional<SessionProxy>,
request: ResourceHandlerRequest<ResourceModel>,
callbackContext: CallbackContext,
logger: LoggerProxy
logger: LoggerProxy,
typeConfiguration: TypeConfigurationModel,
): Promise<ProgressEvent<ResourceModel, CallbackContext>> {
const model = new ResourceModel(request.desiredResourceState);
const progress = ProgressEvent.progress<ProgressEvent<ResourceModel, CallbackContext>>(model);
Expand All @@ -88,14 +94,17 @@ class Resource extends BaseResource<ResourceModel> {
* @param request The request object for the provisioning request passed to the implementor
* @param callbackContext Custom context object to allow the passing through of additional
* state or metadata between subsequent retries
* @param typeConfiguration Configuration data for this resource type, in the given account
* and region
* @param logger Logger to proxy requests to default publishers
*/
@handlerEvent(Action.Delete)
public async delete(
session: Optional<SessionProxy>,
request: ResourceHandlerRequest<ResourceModel>,
callbackContext: CallbackContext,
logger: LoggerProxy
logger: LoggerProxy,
typeConfiguration: TypeConfigurationModel,
Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you mean about changing the order. You guys are the experts on TypeScript here, but curious as to why we cannot use syntax like typeConfiguration?: TypeConfigurationModel here

Choose a reason for hiding this comment

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

i also wondered why ? or Optional<> wasn't used here -- technically I think it should be present on callback and logger as well as typeConfiguration. possibly the types admin null already or the signature isn't needed. but we tried to match existing patterns as much as possible since this isn't our codebase.

what we had wanted to do and did previously was to include the typeConfiguration before the logger, to match the java signature. but that is what caused the problem.

previous versions of the framework code of course call the old signature, ie delete(session, request, callback, logger).

in the test failure we were seeing, that old framework code is running, making that exact call, but against a handler ^ based on the new template, expecting the new signature, so with the changes we had made earlier it thinks the 4th parameter is a typeConfiguration when the caller gave a logger there. (the new framework called the new signature, so if you ran new versions at both places it worked fine.)

in the new code because we haven't changed any existing fields, just appended a new 5th, it quite happily accepts calls from the old framework to new code or even new framework to old code -- in the former case it calls the signature with 4 arguments, and those args match, and it passes undefined in for the typeConfiguration, which works fine at runtime since the TS type info is thrown away when converted to JS; and in the latter case it includes the new 5th argument as a typeConfiguration but old handlers only take 4 args which all match and it just ignores the 5th.

): Promise<ProgressEvent<ResourceModel, CallbackContext>> {
const model = new ResourceModel(request.desiredResourceState);
const progress = ProgressEvent.progress<ProgressEvent<ResourceModel, CallbackContext>>();
Expand All @@ -112,14 +121,17 @@ class Resource extends BaseResource<ResourceModel> {
* @param request The request object for the provisioning request passed to the implementor
* @param callbackContext Custom context object to allow the passing through of additional
* state or metadata between subsequent retries
* @param typeConfiguration Configuration data for this resource type, in the given account
* and region
* @param logger Logger to proxy requests to default publishers
*/
@handlerEvent(Action.Read)
public async read(
session: Optional<SessionProxy>,
request: ResourceHandlerRequest<ResourceModel>,
callbackContext: CallbackContext,
logger: LoggerProxy
logger: LoggerProxy,
typeConfiguration: TypeConfigurationModel,
): Promise<ProgressEvent<ResourceModel, CallbackContext>> {
const model = new ResourceModel(request.desiredResourceState);
// TODO: put code here
Expand All @@ -135,14 +147,17 @@ class Resource extends BaseResource<ResourceModel> {
* @param request The request object for the provisioning request passed to the implementor
* @param callbackContext Custom context object to allow the passing through of additional
* state or metadata between subsequent retries
* @param typeConfiguration Configuration data for this resource type, in the given account
* and region
* @param logger Logger to proxy requests to default publishers
*/
@handlerEvent(Action.List)
public async list(
session: Optional<SessionProxy>,
request: ResourceHandlerRequest<ResourceModel>,
callbackContext: CallbackContext,
logger: LoggerProxy
logger: LoggerProxy,
typeConfiguration: TypeConfigurationModel,
): Promise<ProgressEvent<ResourceModel, CallbackContext>> {
const model = new ResourceModel(request.desiredResourceState);
// TODO: put code here
Expand All @@ -154,7 +169,8 @@ class Resource extends BaseResource<ResourceModel> {
}
}

export const resource = new Resource(ResourceModel.TYPE_NAME, ResourceModel);
// @ts-ignore // if running against v1.0.1 or earlier of plugin the 5th argument is not known but best to ignored (runtime code may warn)
export const resource = new Resource(ResourceModel.TYPE_NAME, ResourceModel, null, null, TypeConfigurationModel)!;

// Entrypoint for production usage after registered in CloudFormation
export const entrypoint = resource.entrypoint;
Expand Down
9 changes: 9 additions & 0 deletions src/exceptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,15 @@ export class NotUpdatable extends BaseHandlerException {}

export class InvalidRequest extends BaseHandlerException {}

export class InvalidTypeConfiguration extends BaseHandlerException {
constructor(typeName: string, reason: string) {
super(
`Invalid TypeConfiguration provided for type '${typeName}'. Reason: ${reason}`,
HandlerErrorCode.InvalidTypeConfiguration
);
}
}

export class AccessDenied extends BaseHandlerException {}

export class InvalidCredentials extends BaseHandlerException {}
Expand Down
2 changes: 2 additions & 0 deletions src/interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ export enum HandlerErrorCode {
ServiceInternalError = 'ServiceInternalError',
NetworkFailure = 'NetworkFailure',
InternalFailure = 'InternalFailure',
InvalidTypeConfiguration = 'InvalidTypeConfiguration',
}

export interface Credentials {
Expand Down Expand Up @@ -261,6 +262,7 @@ export class RequestData<T = Dict> extends BaseDto {
@Expose() providerCredentials?: Credentials;
@Expose() previousResourceProperties?: T;
@Expose() previousStackTags?: Dict<string>;
@Expose() typeConfiguration?: Dict<string>;
}

export class HandlerRequest<ResourceT = Dict, CallbackT = Dict> extends BaseDto {
Expand Down
74 changes: 58 additions & 16 deletions src/resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@ import 'reflect-metadata';
import { boundMethod } from 'autobind-decorator';

import { AwsTaskWorkerPool, ProgressEvent, SessionProxy } from './proxy';
import { BaseHandlerException, InternalFailure, InvalidRequest } from './exceptions';
import {
BaseHandlerException,
InternalFailure,
InvalidRequest,
InvalidTypeConfiguration,
} from './exceptions';
import {
Action,
BaseModel,
Expand Down Expand Up @@ -40,14 +45,17 @@ const MUTATING_ACTIONS: [Action, Action, Action] = [
Action.Delete,
];

export type HandlerSignature<T extends BaseModel> = Callable<
[Optional<SessionProxy>, any, Dict, LoggerProxy],
export type HandlerSignature<
T extends BaseModel,
TypeConfiguration extends BaseModel
> = Callable<
[Optional<SessionProxy>, any, Dict, LoggerProxy, TypeConfiguration],
Promise<ProgressEvent<T>>
>;
export class HandlerSignatures<T extends BaseModel> extends Map<
Action,
HandlerSignature<T>
> {}
export class HandlerSignatures<
T extends BaseModel,
TypeConfiguration extends BaseModel
> extends Map<Action, HandlerSignature<T, TypeConfiguration>> {}
class HandlerEvents extends Map<Action, string | symbol> {}

/**
Expand Down Expand Up @@ -88,7 +96,10 @@ function ensureSerialize<T extends BaseModel>(toResponse = false): MethodDecorat
};
}

export abstract class BaseResource<T extends BaseModel = BaseModel> {
export abstract class BaseResource<
T extends BaseModel = BaseModel,
TypeConfiguration extends BaseModel = BaseModel
> {
protected loggerProxy: LoggerProxy;
protected metricsPublisherProxy: MetricsPublisherProxy;

Expand All @@ -112,10 +123,13 @@ export abstract class BaseResource<T extends BaseModel = BaseModel> {
public readonly typeName: string,
public readonly modelTypeReference: Constructor<T>,
protected readonly workerPool?: AwsTaskWorkerPool,
private handlers?: HandlerSignatures<T>
private handlers?: HandlerSignatures<T, TypeConfiguration>,
public readonly typeConfigurationTypeReference?: Constructor<TypeConfiguration> & {
deserialize: Function;
}
) {
this.typeName = typeName || '';
this.handlers = handlers || new HandlerSignatures<T>();
this.handlers = handlers || new HandlerSignatures<T, TypeConfiguration>();

this.lambdaLogger = console;
this.platformLoggerProxy = new LoggerProxy();
Expand Down Expand Up @@ -294,8 +308,8 @@ export abstract class BaseResource<T extends BaseModel = BaseModel> {

public addHandler = (
action: Action,
f: HandlerSignature<T>
): HandlerSignature<T> => {
f: HandlerSignature<T, TypeConfiguration>
): HandlerSignature<T, TypeConfiguration> => {
this.handlers.set(action, f);
return f;
};
Expand All @@ -304,13 +318,16 @@ export abstract class BaseResource<T extends BaseModel = BaseModel> {
session: Optional<SessionProxy>,
request: BaseResourceHandlerRequest<T>,
action: Action,
callbackContext: Dict
callbackContext: Dict,
typeConfiguration?: TypeConfiguration
): Promise<ProgressEvent<T>> => {
const actionName = action == null ? '<null>' : action.toString();
if (!this.handlers.has(action)) {
throw new Error(`Unknown action ${actionName}`);
}
const handleRequest: HandlerSignature<T> = this.handlers.get(action);
const handleRequest: HandlerSignature<T, TypeConfiguration> = this.handlers.get(
action
);
// We will make the callback context and resource states readonly
// to avoid modification at a later time
deepFreeze(callbackContext);
Expand All @@ -320,7 +337,8 @@ export abstract class BaseResource<T extends BaseModel = BaseModel> {
session,
request,
callbackContext,
this.loggerProxy || this.platformLoggerProxy
this.loggerProxy || this.platformLoggerProxy,
typeConfiguration
);
this.log(`[${action}] handler invoked`);
if (handlerResponse != null) {
Expand Down Expand Up @@ -473,6 +491,27 @@ export abstract class BaseResource<T extends BaseModel = BaseModel> {
}
};

private castTypeConfigurationRequest = (
request: HandlerRequest
): TypeConfiguration => {
try {
if (!this.typeConfigurationTypeReference) {
if (request.requestData.typeConfiguration) {
throw new InternalFailure(
'Type configuration supplied but running with legacy version of code which does not support type configuration.'
);
}
return null;
}
return this.typeConfigurationTypeReference.deserialize(
request.requestData.typeConfiguration
);
} catch (err) {
this.log('Invalid Type Configuration');
throw new InvalidTypeConfiguration(this.typeName, `${err} (${err.name}`);
}
};

// @ts-ignore
public async entrypoint(
eventData: any | Dict,
Expand Down Expand Up @@ -500,6 +539,8 @@ export abstract class BaseResource<T extends BaseModel = BaseModel> {
const [callerCredentials, providerCredentials] = credentials;
const request = this.castResourceRequest(event);

const typeConfiguration = this.castTypeConfigurationRequest(event);

let streamName = `${event.awsAccountId}-${event.region}`;
if (event.stackId && request.logicalResourceIdentifier) {
streamName = `${event.stackId}/${request.logicalResourceIdentifier}`;
Expand Down Expand Up @@ -550,7 +591,8 @@ export abstract class BaseResource<T extends BaseModel = BaseModel> {
this.callerSession,
request,
action,
callback
callback,
typeConfiguration
);
} catch (err) {
error = err;
Expand Down
Loading