fix(AWS API Gateway): Ensure `shouldStartNameWithService` support #10177

vicary · 2021-10-29T20:49:53Z

The closing of #8720 was a workaround by supplying an optional option apiName.

In a perfect world it should correctly deploy with the recommended provider.apiGateway.shouldStartNameWithService without having to provide a custom apiName. But the workaround in the issue make it a required option, otherwise the script exits with code 1, and my CI environment is not happy about it.

This PR is a continuation of the discussion I started after the closing of the issue.

For more context please see #8720 (comment)

pgrzesik

Thanks for contribution @vicary, good catch 👍 I have one suggestion for the implementation that should help avoiding such bugs in the future. Additionally, it would be great to cover this scenario with a test case based on runServerless utility.

pgrzesik · 2021-11-01T14:09:24Z

lib/plugins/aws/package/compile/events/apiGateway/lib/hack/updateStage.js

@@ -150,7 +150,9 @@ async function resolveRestApiId() {
    }
    const apiName = apiGatewayResource
      ? apiGatewayResource.Properties.Name
-      : provider.apiName || `${this.options.stage}-${this.state.service.service}`;
+      : provider.apiName || (provider.apiGateway && provider.apiGateway.shouldStartNameWithService)
+        ? `${this.state.service.service}-${this.options.stage}`


Instead of duplicating the logic, we should rather reuse the naming.getApiGatewayName utility - the reason for this bug is that it was not used previously I believe.

vicary · 2021-11-01T19:22:16Z

@pgrzesik Thanks for the review. Sorry for not super familiar with the internal API yet, using naming.getApiGatewayName() now.

I'll try writing a test with runServerless this week.

pgrzesik · 2021-11-02T11:37:43Z

Perfect @vicary - let me know if you have any questions 💯

codecov · 2021-11-02T11:41:34Z

Codecov Report

Merging #10177 (34bcac5) into master (254e70c) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10177      +/-   ##
==========================================
- Coverage   85.38%   85.35%   -0.04%     
==========================================
  Files         339      339              
  Lines       13819    13831      +12     
==========================================
+ Hits        11800    11805       +5     
- Misses       2019     2026       +7

Impacted Files	Coverage Δ
.../compile/events/apiGateway/lib/hack/updateStage.js	`95.06% <ø> (ø)`
lib/classes/CLI.js	`57.14% <0.00%> (-2.86%)`	⬇️
lib/Serverless.js	`66.96% <0.00%> (-2.51%)`	⬇️
lib/plugins/aws/package/compile/functions.js	`94.44% <0.00%> (ø)`
lib/plugins/aws/provider.js	`94.31% <0.00%> (+0.01%)`	⬆️
lib/plugins/aws/deploy/index.js	`98.68% <0.00%> (+0.05%)`	⬆️
lib/plugins/aws/info/index.js	`97.22% <0.00%> (+0.07%)`	⬆️
lib/utils/telemetry/generatePayload.js	`90.90% <0.00%> (+0.43%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 254e70c...34bcac5. Read the comment docs.

vicary · 2021-11-05T18:58:34Z

@pgrzesik It was quite a trip to stub all the way down to after:deploy:deploy, I've learn a lot more about the inner workings nonetheless. Not sure if there is a quicker way though.

pgrzesik

Looks awesome, I have only one minor suggestion and we should be good to go - well done with mocking all these AWS API calls - this one was definitely very tricky case

pgrzesik · 2021-11-08T10:40:55Z

test/unit/lib/plugins/aws/package/compile/events/apiGateway/lib/hack/updateStage.test.js

@@ -809,4 +809,66 @@ describe('test/unit/lib/plugins/aws/package/compile/events/apiGateway/lib/hack/u
    expect(untagResourceStub).to.have.been.calledOnce;
    expect(untagResourceStub.args[0][0].tagKeys).to.deep.equal(['keytoremove']);
  });
+
+  it('should deploys shouldStartNameWithService without apiName', async () => {


I only have one minor comment - in general, it's better to not override service for tests, as it's automatically generated - I see that here we need service for assertion so we can go this route instead:

const { serviceConfig, servicePath: serviceDir, updateConfig } = await fixtures.setup('apiGateway'); // Here we can get service from serviceConfig await updateConfig(); // Here we can pass config ext content await runServerless({cwd: serviceDir, ...}) // change fixture to cwd

Here's a partial example on how fixtures are used along with runServerless:

serverless/test/unit/lib/plugins/package/lib/packageService.test.js

Line 306 in ecf2918

const { servicePath: serviceDir, updateConfig } = await fixtures.setup('packageArtifact');

Should I also replace my const service = "..."; with serviceConfig.service?

yes, I don't remember exactly if under service there will be name or an object so you might need to check if serviceConfig.service.name won't be needed there

It's a plain string, I've update the test.

pgrzesik

Sorry, Ive missed one minor thing and we're good to go

pgrzesik · 2021-11-08T14:04:53Z

lib/plugins/aws/package/compile/events/apiGateway/lib/hack/updateStage.js

@@ -150,7 +150,7 @@ async function resolveRestApiId() {
    }
    const apiName = apiGatewayResource
      ? apiGatewayResource.Properties.Name
-      : provider.apiName || `${this.options.stage}-${this.state.service.service}`;
+      : provider.apiName || provider.naming.getApiGatewayName();


I've missed one thing here - provider.apiName is already resolved inside getApiGatewayName so let's simplify this and we're good to go

pgrzesik

Thank you @vicary, looks great 👍

SrushithR · 2021-11-10T08:43:53Z

Hi @vicary @pgrzesik. Since the latest release (2.66.0), our deployments with serverless pro have started failing. The same stack without the serverless pro details deploys without any issues.

For now, I have downgraded the serverless package version to 2.65.0 and the deployments are working fine. Can you please help me with this?

pgrzesik · 2021-11-10T09:04:42Z

Hello @SrushithR - could you please provide more details on how your deployments are failing? If you could share the error that you're experiencing that would be great

vicary · 2021-11-10T09:07:17Z

If this is reproducible and confirmed related, I'll make a regression test.

pgrzesik · 2021-11-10T09:30:04Z

I think I've found the issue with the change - I'll prepare a PR with the fix and release shortly after it's accepted

SrushithR · 2021-11-10T09:40:04Z

@pgrzesik Great. I have created an issue regarding the same - #10220

fixes serverless#8720

60c93f9

pgrzesik suggested changes Nov 1, 2021

View reviewed changes

pgrzesik added bug cat/aws-event-api-gateway labels Nov 1, 2021

pgrzesik assigned vicary Nov 1, 2021

use shared util for apigw name

b738edf

unit test

4fe9cb5

pgrzesik suggested changes Nov 8, 2021

View reviewed changes

vicary added 2 commits November 8, 2021 19:44

replace configExt with fixtures

7d0ae10

remove .only()

00235c0

pgrzesik suggested changes Nov 8, 2021

View reviewed changes

api name resolution logic dedupe

34bcac5

pgrzesik approved these changes Nov 8, 2021

View reviewed changes

pgrzesik changed the title ~~fixes #8720~~ fix(AWS API Gateway): Ensure shouldStartNameWithService support Nov 8, 2021

pgrzesik merged commit e8c8d25 into serverless:master Nov 8, 2021

pgrzesik mentioned this pull request Nov 10, 2021

fix(AWS API Gateway): Ensure proper apiId resolution #10221

Merged

vicary deleted the patch-1 branch December 2, 2021 13:28

vicary mentioned this pull request Dec 2, 2021

Add integration tests serverless-heaven/serverless-webpack#1019

Closed

vicary mentioned this pull request Mar 10, 2022

Supports NPM 8 and convert tests from Mocha to Jest serverless-heaven/serverless-webpack#1084

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(AWS API Gateway): Ensure `shouldStartNameWithService` support #10177

fix(AWS API Gateway): Ensure `shouldStartNameWithService` support #10177

vicary commented Oct 29, 2021 •

edited

pgrzesik left a comment

pgrzesik Nov 1, 2021

vicary commented Nov 1, 2021

pgrzesik commented Nov 2, 2021

codecov bot commented Nov 2, 2021 •

edited

vicary commented Nov 5, 2021

pgrzesik left a comment

pgrzesik Nov 8, 2021 •

edited

vicary Nov 8, 2021

pgrzesik Nov 8, 2021

vicary Nov 8, 2021

pgrzesik left a comment

pgrzesik Nov 8, 2021

vicary Nov 8, 2021

pgrzesik left a comment

SrushithR commented Nov 10, 2021

pgrzesik commented Nov 10, 2021 •

edited

vicary commented Nov 10, 2021

pgrzesik commented Nov 10, 2021

SrushithR commented Nov 10, 2021 •

edited

fix(AWS API Gateway): Ensure shouldStartNameWithService support #10177

fix(AWS API Gateway): Ensure shouldStartNameWithService support #10177

Conversation

vicary commented Oct 29, 2021 • edited

pgrzesik left a comment

Choose a reason for hiding this comment

pgrzesik Nov 1, 2021

Choose a reason for hiding this comment

vicary commented Nov 1, 2021

pgrzesik commented Nov 2, 2021

codecov bot commented Nov 2, 2021 • edited

Codecov Report

vicary commented Nov 5, 2021

pgrzesik left a comment

Choose a reason for hiding this comment

pgrzesik Nov 8, 2021 • edited

Choose a reason for hiding this comment

vicary Nov 8, 2021

Choose a reason for hiding this comment

pgrzesik Nov 8, 2021

Choose a reason for hiding this comment

vicary Nov 8, 2021

Choose a reason for hiding this comment

pgrzesik left a comment

Choose a reason for hiding this comment

pgrzesik Nov 8, 2021

Choose a reason for hiding this comment

vicary Nov 8, 2021

Choose a reason for hiding this comment

pgrzesik left a comment

Choose a reason for hiding this comment

SrushithR commented Nov 10, 2021

pgrzesik commented Nov 10, 2021 • edited

vicary commented Nov 10, 2021

pgrzesik commented Nov 10, 2021

SrushithR commented Nov 10, 2021 • edited

fix(AWS API Gateway): Ensure `shouldStartNameWithService` support #10177

fix(AWS API Gateway): Ensure `shouldStartNameWithService` support #10177

vicary commented Oct 29, 2021 •

edited

codecov bot commented Nov 2, 2021 •

edited

pgrzesik Nov 8, 2021 •

edited

pgrzesik commented Nov 10, 2021 •

edited

SrushithR commented Nov 10, 2021 •

edited