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.

Sign up for GitHub

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

Jump to bottom

Support API Gateway stage deployment description #5509

Merged
merged 14 commits into from
Mar 7, 2019
Merged

Support API Gateway stage deployment description #5509

merged 14 commits into from
Mar 7, 2019

Conversation

vkkis93
Copy link
Contributor

@vkkis93 vkkis93 commented Nov 20, 2018 •
edited
Loading

What did you implement:

Closes #5400

How did you implement it:

Allowed to be used property Description in AWS::ApiGateway::Deployment

How can we verify it:

Need to add field description in

  apiGateway:
    restApiId: xxxxxxxxxx
    restApiRootResourceId: xxxxxxxxxx
    description: HelloWorld

and after deployment - description will be added to deployment histrory tab

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

softprops
softprops reviewed Nov 21, 2018
View reviewed changes
lib/plugins/aws/package/compile/events/apiGateway/lib/deployment.js
@@ -14,6 +14,7 @@ module.exports = {
Properties: {
RestApiId: this.provider.getApiGatewayRestApiId(),
StageName: this.provider.getStage(),
Description: this.provider.getApiGatewayDescription(),
Copy link
Contributor

@softprops softprops Nov 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Speaking up because I thing this feature will actually fix #3495 (comment) if implemented correctly.

The cloud formation resource for this is not a string but a structured type
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-apigateway-deployment.html

It's the structured type that enables turning on active tracing for the gateway https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-apigateway-deployment-stagedescription.html

Copy link
Contributor

Choose a reason for hiding this comment

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

I take that back. That's StageDescription vs Description. You may get more bang for your buck if you add them both here. I'd but happy to add a follow pull that just mirrors this with StageDescription if that's too much scope.

Copy link
Contributor Author

@vkkis93 vkkis93 Nov 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I can add StageDescription
I will create new PR which will be based on this one

UPD: I have looked on stage description and it's very complex task.
I would like to do it but not right now.
Can we finish the first with this PR ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer if it only sets the description if it's provided instead of setting it to an empty string if it's not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dschep I've done

@softprops
Copy link
Contributor

Any eta on this? I looks pretty solid to me

dschep
dschep previously requested changes Nov 29, 2018
View reviewed changes
Copy link
Contributor

@dschep dschep 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 with one small change request.

lib/plugins/aws/package/compile/events/apiGateway/lib/deployment.js
@@ -14,6 +14,7 @@ module.exports = {
Properties: {
RestApiId: this.provider.getApiGatewayRestApiId(),
StageName: this.provider.getStage(),
Description: this.provider.getApiGatewayDescription(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer if it only sets the description if it's provided instead of setting it to an empty string if it's not

@dschep dschep self-assigned this Nov 29, 2018
@softprops softprops mentioned this pull request Jan 13, 2019
Merged
7 tasks
@pmuens pmuens assigned pmuens and unassigned dschep Mar 7, 2019
pmuens
pmuens approved these changes Mar 7, 2019
View reviewed changes
Copy link
Contributor

@pmuens pmuens 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 working on this @vkkis93 👍

I just pulled this today and tested it thorougly. Works like a charm. Merged master into it so that it's up to date and pushed some doc updates.

Will merge once Travis says everything is fine :shipit:

@pmuens pmuens dismissed dschep’s stale review March 7, 2019 12:51

Stale review

@pmuens pmuens merged commit 825f20c into serverless:master Mar 7, 2019
@pmuens pmuens added this to the 1.39.0 milestone Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@softprops softprops softprops left review comments

@pmuens pmuens pmuens approved these changes

@dschep dschep dschep left review comments

Assignees

@pmuens pmuens

Labels
enhancement
Projects
None yet
Milestone
1.39.0
Development

Successfully merging this pull request may close these issues.
4 participants
@vkkis93 @softprops @dschep @pmuens