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(aws-apigateway): expand RestApi support to models, parameters and validators #2960

Merged
merged 17 commits into from
Jun 27, 2019
Merged

feat(aws-apigateway): expand RestApi support to models, parameters and validators #2960

merged 17 commits into from
Jun 27, 2019

Conversation

julienlepine
Copy link
Contributor

@julienlepine julienlepine commented Jun 20, 2019

Fixes #905: "apigateway: "methodResponses" is missing from MethodOptions"
Fixes #1695: apigateway: missing support for models
Fixes #727: API Gateway: improve API for request parameters and responses
Fixes #723: API Gateway: missing features
Fixes #2957: RestApi to use logical id as a name for APIs instead of name of current construct
Adds support for JsonSchema in Model
Aligns Model to the PhysicalName convention.
No breaking change, documentation updated


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
    • Design: For significant features, design document added to design folder
  • Title and Description
    • Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

…d validators

Fixes #905: "apigateway: "methodResponses" is missing from MethodOptions"
Fixes #1695: apigateway: missing support for models
Fixes #727: API Gateway: improve API for request parameters and responses
Fixes #723: API Gateway: missing features
Fixes #2957: RestApi to use logical id as a name for APIs instrea of name of current construct
…d validators

Fixes #905: "apigateway: "methodResponses" is missing from MethodOptions"
Fixes #1695: apigateway: missing support for models
Fixes #727: API Gateway: improve API for request parameters and responses
Fixes #723: API Gateway: missing features
Fixes #2957: RestApi to use logical id as a name for APIs instrea of name of current construct
@julienlepine julienlepine requested a review from a team as a code owner June 20, 2019 13:25
@julienlepine
Copy link
Contributor Author

re-aligned with the changes to have the PhysicalName as the default name of the construct, merged with the incoming changes.

@NGL321
Copy link
Contributor

NGL321 commented Jun 21, 2019

Hi Julien! Thank you so much for contributing. We are working hard to stabilize the CDK APIs and tuning them to meet our consistency guidelines. While we work on getting the APIs aligned with our guidelines, we are pausing work on most community PRs starting today. Please continue to report issues and submit feature requests, of course. We expect to get back to work on community PRs within a few weeks.

packages/@aws-cdk/aws-apigateway/lib/method.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigateway/lib/model.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigateway/lib/model.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigateway/lib/model.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigateway/lib/model.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigateway/lib/requestvalidator.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigateway/lib/requestvalidator.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigateway/lib/requestvalidator.ts Outdated Show resolved Hide resolved
@julienlepine
Copy link
Contributor Author

Re merged with the latest master commit, and adressed PR comments.

@julienlepine
Copy link
Contributor Author

Merged with 0.36

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Looks good, getting there.

A few notes:

  1. We can't introduce breaking changes any more to any of the APIs, so we'll have to figure out the right way to maintain compat.
  2. Check out the new pattern we have for allowing physical names.

packages/@aws-cdk/aws-apigateway/lib/model.ts Outdated Show resolved Hide resolved

constructor(scope: Construct, id: string, props: ModelProps) {
super(scope, id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Follow the new pattern to support physical names (see s3.Bucket for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Use PhysicalName constructs
@julienlepine
Copy link
Contributor Author

Updated based on comments

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Getting there! Super high quality contribution. Thanks so much for all the effort.

packages/@aws-cdk/aws-apigateway/lib/model.ts Outdated Show resolved Hide resolved
export interface ModelOptions {
/**
* The content type for the model.
* @default None
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate in the docs what does it mean not to provide a content type?

Also, the value in @default should be “-“ to indicate it’s not a “real” value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

packages/@aws-cdk/aws-apigateway/lib/model.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigateway/lib/model.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigateway/lib/json-schema.ts Outdated Show resolved Hide resolved
physicalName: props.requestValidatorName,
});

const validatorProps: CfnRequestValidatorProps = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be “this.physicalName”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done for Model.ts, for RequestValidator the name is actually kind of useless per se, name is not a property of the CfnRequestValidator, and it's really an Id.

@@ -601,4 +601,34 @@ export = {

test.done();
},

Copy link
Contributor

Choose a reason for hiding this comment

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

What about request validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

Completing migration to PhysicalName
Moved JsonSchemaMapper to internal class in util.ts
Removed dependency to IConstruct on IModel (and added linter exclusion in package.json)
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Looks great. Please merge from master, update PR description and let me know when you are ready for merge.

packages/@aws-cdk/aws-apigateway/lib/model.ts Show resolved Hide resolved
@julienlepine
Copy link
Contributor Author

Merged and pushed

@julienlepine
Copy link
Contributor Author

Ready to be merged to master.

@eladb
Copy link
Contributor

eladb commented Jun 26, 2019

Can you please update the PR description so we’ll use it for the merge commit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment