feat(api-gateway): allow configuration of deployment description #21207

josephedward · 2022-07-18T16:32:08Z

Closes #20934

Motivation: Customer would like to be able to set the description per deployment. From inside their pipeline, they could get the commit hash / commit message, timestamp, custom text, and other git-related metadata that they would like to set as description.

Thanks to @TheRealAmazonKendra for help cleaning this PR up and providing some pointers on locally building the CDK.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

aws#20763

remove trailing spaces

gitpod-io · 2022-07-18T16:32:15Z

josephedward · 2022-07-18T16:46:18Z

@peterwoodworth since RestApi already has a description (it was just never passed to _configureDeployment) do you think this warrants a change to a test or the README? This PR is for #20934 - but I don't see where _configureDeployment or the RestApiBaseProps interface are tested.

TheRealAmazonKendra

Thank you for your contribution. Please see my comment below. We will provide a more thorough review once the build is fixed and tests have been written.

TheRealAmazonKendra · 2022-07-18T19:05:50Z

packages/@aws-cdk/aws-apigateway/lib/restapi.ts

@@ -554,7 +561,7 @@ export abstract class RestApiBase extends Resource implements IRestApi {
    if (deploy) {

      this._latestDeployment = new Deployment(this, 'Deployment', {
-        description: 'Automatically created by the RestApi construct',
+        description: props.description? props.description :'Automatically created by the RestApi construct',


Take a closer look at where this function is called. It's in two constructors, SpecRestApi and RestApi. The props for one of these doesn't have the description field. You'll need to add it there and then add some tests in both places for the output of description being set and not being set.

I will exempt a README change for this but it will require an update to the expected output of the integ tests as this will change the template.

Thank you @TheRealAmazonKendra ! That raises another question I had - how do I update those integration test outputs? Do I have to run a CDK synth, and if so, what do I need to pass to it?

See INTEGRATION_TESTS.md

If RestApiProps has a description var, and SpecRestApiProps extends RestApiBaseProps(where I added the var), do I need to add code to those constructors? Is it problematic that this description would possibly overlap with a description in the OpenAPI specification?

I interpreted the both the error message and the code wrong. Scratch everything I said and sorry for the confusion on my part.

The issue here is that RestApiProps extends RestApiOptions which extends RestApiBaseProps. So, description is being declared twice in the context of RestApiProps. Since you are (correctly) adding it to RestApiBaseProps it should be removed from RestApiProps. I think that will solve the error.

TheRealAmazonKendra · 2022-07-19T17:35:48Z

Two additional quick notes:

This PR needs a body that aligns with our guidelines in the Pull Request section of the contributing guide. Please review that and revise the PR body. The title is great and needs no update.

Since you're not writing new integration tests, you don't really have to do the whole process in the file I linked to. Just run the build, see where the failures are, and update the expected values in those files as long as it aligns with what you'd expect to see as output.

You can either push changes and let CodeBuild run until failure and push another change to fix that failure (this will likely take a few attempts) or you can do it locally. If you do it locally, I highly recommend using the scrips/foreach.sh version of the build. Basically, build everything (warning: this takes a long time) using yarn install && scripts/foreach.sh yarn build. If a package fails the build for whatever reason (sometimes there's just a fluke failure) you can start where it left off with scripts/foreach.sh yarn build again.

Once the builds are complete, run scripts/foreach.sh yarn test. When you hit a failure, fix the output and re-run that. It starts from that package again so you're not starting over at the beginning. This is what I've found to be the fastest and most efficient way to find all the packages that have impacted integration tests.

…loyment-description

Pull request has been modified.

josephedward · 2022-07-20T17:20:45Z

Thanks for the helpful info - running the full build locally now, but I committed that change and edited the PR body. I don't think I can see anything from CodeBuild? It wants me to authenticate first.

…loyment-description

TheRealAmazonKendra

Thanks for the helpful info - running the full build locally now, but I committed that change and edited the PR body. I don't think I can see anything from CodeBuild? It wants me to authenticate first.

You can read the output once the build completes. In this case, no integ test updates were needed so I'll exempt that. Please do add tests for both the case when you add a description and when you don't. Once we get that, I see this as done.

added tests for: - instantiating RestApi with description - instantiating RestAPI with no description - calling _configureDeployment with a custom description - calling _configureDeployment with no description

…loyment-description

Pull request has been modified.

josephedward · 2022-07-22T15:10:11Z

I added tests for creating a RestApi with and without a description, and tests for calling _configureDeployment with and without a description (since calling _configureDeployment defaults to 'Automatically created by the RestApi construct' instead of being blank). Linter still wants an update to the integration tests - I was running those but wasn't sure if you wanted me to update the snapshots, then I dropped it when I saw your message.

Removed double quotations

…cription' of https://github.com/josephedward/aws-cdk into feat(api-gateway)-allow-configuration-of-deployment-description

removing _configureDeployment

TheRealAmazonKendra · 2022-07-22T17:00:13Z

Oooops. You're totally right that I said I'd exempt integ test updates and then forgot to. It is possible, though, that the output will be impacted and an update will need to be made to the snapshots. Without looking through the tests themselves, I can't say for sure.

TheRealAmazonKendra · 2022-07-22T17:01:16Z

packages/@aws-cdk/aws-ssm/lib/parameter.ts

@@ -333,6 +333,23 @@ export interface SecureStringParameterAttributes extends CommonStringParameterAt
 * @example
 *
 * const ssmParameter = new ssm.StringParameter(this, 'mySsmParameter', {
+<<<<<<< HEAD


Looks like the merge went wrong here.

Sorry about that! I must have pulled from main on my local fork.

trailing spaces and semicolon

…loyment-description

mergify · 2022-07-25T21:01:31Z

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

…loyment-description

aws-cdk-automation · 2022-07-25T21:37:34Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: c6c7bd0
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-07-25T21:37:58Z

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

@TheRealAmazonKendra

…#21207) ---- Closes aws#20934 Motivation: Customer would like to be able to set the description per deployment. From inside their pipeline, they could get the commit hash / commit message, timestamp, custom text, and other git-related metadata that they would like to set as description. Thanks to @TheRealAmazonKendra for help cleaning this PR up and providing some pointers on locally building the CDK. ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

@TheRealAmazonKendra

…#21207) ---- Closes aws#20934 Motivation: Customer would like to be able to set the description per deployment. From inside their pipeline, they could get the commit hash / commit message, timestamp, custom text, and other git-related metadata that they would like to set as description. Thanks to @TheRealAmazonKendra for help cleaning this PR up and providing some pointers on locally building the CDK. ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

@TheRealAmazonKendra

…#21207) ---- Closes aws#20934 Motivation: Customer would like to be able to set the description per deployment. From inside their pipeline, they could get the commit hash / commit message, timestamp, custom text, and other git-related metadata that they would like to set as description. Thanks to @TheRealAmazonKendra for help cleaning this PR up and providing some pointers on locally building the CDK. ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

@TheRealAmazonKendra

…#21207) ---- Closes aws#20934 Motivation: Customer would like to be able to set the description per deployment. From inside their pipeline, they could get the commit hash / commit message, timestamp, custom text, and other git-related metadata that they would like to set as description. Thanks to @TheRealAmazonKendra for help cleaning this PR up and providing some pointers on locally building the CDK. ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

josephedward and others added 11 commits June 22, 2022 19:15

fix:(aws-ssm) ssm: StringParameter example is wrong aws#20763

707e18c

aws#20763

Merge branch 'main' into main

036a255

docs(aws-ssm): ssm: StringParameter example is wrong

8c7a004

remove trailing spaces

Merge branch 'main' into main

e3ce26b

Merge branch 'aws:main' into main

918082e

Merge branch 'aws:main' into main

1d1ba88

docs(aws-docdb) Incorrect enum in sample snippet

46b2df4

Merge branch 'main' of https://github.com/josephedward/aws-cdk

d5b1af9

Merge branch 'main' into main

5619665

Merge branch 'aws:main' into main

22038c4

Update restapi.ts

bc08abb

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jul 18, 2022

aws-cdk-automation requested a review from a team July 18, 2022 16:32

josephedward mentioned this pull request Jul 18, 2022

(api-gateway): allow configuration of deployment description #20934

Closed

2 tasks

TheRealAmazonKendra previously requested changes Jul 18, 2022

View reviewed changes

josephedward added 2 commits July 18, 2022 22:29

Merge branch 'main' of https://github.com/aws/aws-cdk into aws-main

2fcf2f5

Merge branch 'aws-main'

ebef375

TheRealAmazonKendra added the pr-linter/exempt-readme The PR linter will not require README changes label Jul 19, 2022

josephedward and others added 2 commits July 20, 2022 13:11

Update restapi.ts

875f735

Merge branch 'main' into feat(api-gateway)-allow-configuration-of-dep…

308f1ae

…loyment-description

josephedward and others added 3 commits July 20, 2022 14:50

Merge branch 'aws:main' into main

d4a3e78

Merge branch 'main' into feat(api-gateway)-allow-configuration-of-dep…

3d7d468

…loyment-description

Merge branch 'main' into feat(api-gateway)-allow-configuration-of-dep…

3828be1

…loyment-description

TheRealAmazonKendra previously requested changes Jul 21, 2022

View reviewed changes

josephedward and others added 2 commits July 22, 2022 11:03

Update restapi.test.ts

e13f789

added tests for: - instantiating RestApi with description - instantiating RestAPI with no description - calling _configureDeployment with a custom description - calling _configureDeployment with no description

Merge branch 'main' into feat(api-gateway)-allow-configuration-of-dep…

bf76bec

…loyment-description

josephedward added 4 commits July 22, 2022 11:33

Update restapi.test.ts

293a785

Removed double quotations

Merge branch 'feat(api-gateway)-allow-configuration-of-deployment-des…

637822f

…cription' of https://github.com/josephedward/aws-cdk into feat(api-gateway)-allow-configuration-of-deployment-description

Update restapi.test.ts

8ccc6ef

Update restapi.test.ts

667bc48

removing _configureDeployment

TheRealAmazonKendra added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jul 22, 2022

TheRealAmazonKendra reviewed Jul 22, 2022

View reviewed changes

josephedward and others added 5 commits July 22, 2022 13:05

Update parameter.ts

6121bb0

Update restapi.test.ts

a610a19

trailing spaces and semicolon

Merge branch 'main' into feat(api-gateway)-allow-configuration-of-dep…

68d5b42

…loyment-description

Update restapi.test.ts

2f40afb

Merge branch 'main' into feat(api-gateway)-allow-configuration-of-dep…

0f735a5

…loyment-description

TheRealAmazonKendra approved these changes Jul 25, 2022

View reviewed changes

Merge branch 'main' into feat(api-gateway)-allow-configuration-of-dep…

c6c7bd0

…loyment-description

mergify bot merged commit 03fc2bd into aws:main Jul 25, 2022

akash1810 mentioned this pull request Aug 8, 2022

fix(deps): Update aws-cdk-lib to 2.35.0 guardian/cdk#1408

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api-gateway): allow configuration of deployment description #21207

feat(api-gateway): allow configuration of deployment description #21207

josephedward commented Jul 18, 2022 •

edited

Loading

gitpod-io bot commented Jul 18, 2022

josephedward commented Jul 18, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra Jul 18, 2022

josephedward Jul 19, 2022

TheRealAmazonKendra Jul 19, 2022

josephedward Jul 19, 2022 •

edited

Loading

TheRealAmazonKendra Jul 19, 2022 •

edited

Loading

TheRealAmazonKendra commented Jul 19, 2022

josephedward commented Jul 20, 2022

TheRealAmazonKendra left a comment

josephedward commented Jul 22, 2022

TheRealAmazonKendra commented Jul 22, 2022

TheRealAmazonKendra Jul 22, 2022

josephedward Jul 22, 2022

mergify bot commented Jul 25, 2022

aws-cdk-automation commented Jul 25, 2022

mergify bot commented Jul 25, 2022

feat(api-gateway): allow configuration of deployment description #21207

feat(api-gateway): allow configuration of deployment description #21207

Conversation

josephedward commented Jul 18, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Jul 18, 2022

josephedward commented Jul 18, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Jul 18, 2022

Choose a reason for hiding this comment

josephedward Jul 19, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra Jul 19, 2022

Choose a reason for hiding this comment

josephedward Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

TheRealAmazonKendra Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

TheRealAmazonKendra commented Jul 19, 2022

josephedward commented Jul 20, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

josephedward commented Jul 22, 2022

TheRealAmazonKendra commented Jul 22, 2022

TheRealAmazonKendra Jul 22, 2022

Choose a reason for hiding this comment

josephedward Jul 22, 2022

Choose a reason for hiding this comment

mergify bot commented Jul 25, 2022

aws-cdk-automation commented Jul 25, 2022

AWS CodeBuild CI Report

mergify bot commented Jul 25, 2022

josephedward commented Jul 18, 2022 •

edited

Loading

josephedward Jul 19, 2022 •

edited

Loading

TheRealAmazonKendra Jul 19, 2022 •

edited

Loading