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

chore(apigateway): cleanup DomainName TLS docstring and tests #13309

Merged
merged 10 commits into from
Mar 3, 2021

Conversation

robertd
Copy link
Contributor

@robertd robertd commented Feb 27, 2021

Cleanup of apigateway module (docs & tests).


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

@gitpod-io
Copy link

gitpod-io bot commented Feb 27, 2021

@github-actions github-actions bot added the @aws-cdk/aws-apigateway Related to Amazon API Gateway label Feb 27, 2021
@robertd robertd changed the title chore(apigateway): change default SecurityPolicy for DomainName to TLS_1_2 fix(apigateway): change default SecurityPolicy for DomainName to TLS_1_2 Feb 28, 2021
nija-at
nija-at previously requested changes Mar 3, 2021
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this change @robertd.

I believe the default that is documented is the default that CloudFormation is applying.
I've not personally verified this, and if that's inaccurate, we need to update the doc string.

@aws-cdk/aws-apigateway is a stable module and we don't accept breaking changes to this module. Unfortunately, the default will have to remain what it already is.

@mergify mergify bot dismissed nija-at’s stale review March 3, 2021 15:47

Pull request has been modified.

@robertd
Copy link
Contributor Author

robertd commented Mar 3, 2021

@nija-at I've removed portions of setting the TLS by default. In the CloudFormation docs SecurityPolicy is an optional parameter. I'll try creating a vanilla api gateway and see what it defaults to. In the meantime... I'm changing this PR to be more of a chore/cleanup one.

I'll provide an update soon.

@robertd
Copy link
Contributor Author

robertd commented Mar 3, 2021

@nija-at It seems like CloudFormation is setting this to TLS_1_0 by default. I'll update the docstring to reflect this.

image

@robertd
Copy link
Contributor Author

robertd commented Mar 3, 2021

@nija-at I've repurposed this PR as a chore/cleanup. I'm not really sure where to submit a PR to update cloudformation docs.

Also, I still think this PR addresses #13306 to a certain extent since we cannot set default value due to module being stable at this point.

Thanks!

nija-at
nija-at previously requested changes Mar 3, 2021
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

The rest of the code clean up looks good. See 2 comments below.

packages/@aws-cdk/aws-apigateway/lib/domain-name.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-apigateway/test/domains.test.ts Outdated Show resolved Hide resolved
@nija-at
Copy link
Contributor

nija-at commented Mar 3, 2021

Could you also update the PR title and description to match your new updates?

@robertd robertd changed the title fix(apigateway): change default SecurityPolicy for DomainName to TLS_1_2 chore(apigateway): cleanup DomainName TLS docstring and tests Mar 3, 2021
@robertd
Copy link
Contributor Author

robertd commented Mar 3, 2021

Could you also update the PR title and description to match your new updates?

Updated the title... forgot to hit Save button :)

@mergify mergify bot dismissed nija-at’s stale review March 3, 2021 18:25

Pull request has been modified.

@robertd
Copy link
Contributor Author

robertd commented Mar 3, 2021

@nija-at reverts have been made...

@mergify
Copy link
Contributor

mergify bot commented Mar 3, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: d5c71e4
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Mar 3, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 2c074de into aws:master Mar 3, 2021
@robertd robertd deleted the robertd/api-gateway-tls branch March 4, 2021 05:17
cornerwings pushed a commit to cornerwings/aws-cdk that referenced this pull request Mar 8, 2021
)

Cleanup of apigateway module (docs & tests).

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants