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

AWS: Add API Gateway stage name validation. #5639

Merged
merged 7 commits into from Jan 4, 2019

Conversation

Projects
None yet
5 participants
@exoego
Copy link
Contributor

exoego commented Dec 30, 2018

What did you implement:

Closes #4737

How did you implement it:

Add hyphen validation to AwsProvider constructor, so users can get informed about what is invalid stage name before deploy which is time-consuming.

  • Hyphen in stage name will be an error only if http events are present, since API Gateway does not support hyphen in stage name.
  • Why validation in constructor? Because I think it can raise error in both createStack and updateStack time.

Underscore is already implemented in
https://github.com/serverless/serverless/blob/3c1b4393af2416eff5ce269fd187363512ede/lib/plugins/aws/deploy/lib/createStack.js#L50-L57 so this PR does not touch underscore.

How can we verify it:

Run test.

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

exoego added some commits Dec 30, 2018

@horike37

This comment has been minimized.

Copy link
Member

horike37 commented Dec 31, 2018

@exoego
This would be a breaking change for some users who don't use API Gateway and already include hyphens in stagename since those users can't deploy after upgrading sls version.
We might want to separate a way how to check stage name whether http event is in serverless.yml.
What do you think?

@horike37

This comment has been minimized.

Copy link
Member

horike37 commented Dec 31, 2018

how about just showing a warning message when hyphens and underscores are in stage name without stopping serverless process?

@exoego

This comment has been minimized.

Copy link
Contributor

exoego commented Dec 31, 2018

This would be a breaking change for some users who don't use API Gateway and already include hyphens in stagename since those users can't deploy after upgrading sls version.
We might want to separate a way how to check stage name whether http event is in serverless.yml.
What do you think?

Agree.
Hyphen should be error only if API gateway events are in the service definition.
Underscore should be error always since CloudFront does not support it.

how about just showing a warning message when hyphens and underscores are in stage name without stopping serverless process?

I thinks stopping process is preferrable than warning. Since users can get informed before deploy, which is time consuming.

@horike37

This comment has been minimized.

Copy link
Member

horike37 commented Jan 4, 2019

@exoego
Thank you for updating validation for http event 👍 Looks good 😄

Underscore should be error always since CloudFront does not support it.

I think this idea still be a breaking change as well. there should be people who already use underscore as stagename. Is it possible to avoid it? e.g. doing validation only when detecting if CloudFront resource is used from serverless.yml.

@horike37 horike37 added pr/in-review and removed needs feedback labels Jan 4, 2019

@exoego

This comment has been minimized.

Copy link
Contributor

exoego commented Jan 4, 2019

Underscore should be error always since CloudFront does not support it.

I think this idea still be a breaking change as well. there should be people who already use underscore as stagename. Is it possible to avoid it? e.g. doing validation only when detecting if CloudFront resource is used from serverless.yml.

I think restring underscore is not a breaking change, since it is already implemented in

const errorMessage = [
`The stack service name "${stackName}" is not valid. `,
'A service name should only contain alphanumeric',
' (case sensitive) and hyphens. It should start',
' with an alphabetic character and shouldn\'t',
' exceed 128 characters.',
].join('');
throw new this.serverless.classes.Error(errorMessage);

BTW, I did not notice there is already validation until now.
I will try to consolidate my implementation into the original validation.

Since underscore is validated when creating stack, I will remove it from my code.

@horike37

This comment has been minimized.

Copy link
Member

horike37 commented Jan 4, 2019

@exoego

I think restring underscore is not a breaking change, since it is already implemented in

Ah 😅 you are right. I didn't know that too.

I will try to consolidate my implementation into the original validation.

Great 👍 Ping us if you are ready for review again 😄

@exoego

This comment has been minimized.

Copy link
Contributor

exoego commented Jan 4, 2019

@horike37
Removed the redundant underscore validation !!

@horike37
Copy link
Member

horike37 left a comment

@exoego
That's a super awesome 👍 Looks perfect from my perspective 💯

@horike37 horike37 merged commit cfd6c62 into serverless:master Jan 4, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 90.829%
Details

@exoego exoego deleted the exoego:aws-stage-validation branch Jan 4, 2019

@exoego exoego changed the title Add stage name validation to AWS provider. Add API Gateway stage name validation to AWS provider. Jan 4, 2019

@exoego exoego changed the title Add API Gateway stage name validation to AWS provider. AWS: Add API Gateway stage name validation. Jan 4, 2019

dschep added a commit that referenced this pull request Jan 9, 2019

v1.36.0 release!
 - [Log AWS SDK calls in debug mode](#5604)
 - [Added currently supported regions for GCP functions](#5601)
 - [Update Cloudflare Templates](#5620)
 - [AWS: Validate rate/cron syntax before Deploy](#5635)
 - [Fix error log output](#5378)
 - [Support for native async/await in AWS Lambda for aws-nodejs-typescript template ](#5607)
 - [aws-csharp create template uses handler-specific artifact](#5411)
 - [change behaviour on initial stack create failed](#5631)
 - [Add warning for multiple functions having same handler](#5638)
 - [AWS: Add API Gateway stage name validation.](#5639)

@dschep dschep referenced this pull request Jan 9, 2019

Merged

v1.36.0 release! #5670

@shortjared shortjared added this to the 1.36.0 milestone Jan 9, 2019

@zorrodg

This comment has been minimized.

Copy link

zorrodg commented Jan 11, 2019

Hello guys, just FYI: I believe this change introduced a bug when stage is referenced in serverless.yml as a variable, like this: https://serverless.com/framework/docs/providers/aws/guide/variables#recursively-reference-properties.

I'm seeing the following error:

Serverless plugin ".../node_modules/serverless/lib/plugins/aws/provider/awsProvider.js" initialization e
rrored: Invalid stage name ${opt:stage, 'dev'}: it should contains only [a-zA-Z0-9] for AWS provider if http event are present since API Gateway stage name cannot con
tains hyphens.

Not sure if already captured. If so, can you please point me to the bug #?

Thanks!

@Anonymike

This comment has been minimized.

Copy link

Anonymike commented Jan 11, 2019

Hey @exoego I was just able to create a stage in API Gateway with a hyphen. Is this set to be phased out?

https://imgur.com/qGBqAYi

@exoego

This comment has been minimized.

Copy link
Contributor

exoego commented Jan 11, 2019

@Anonymike
Yes, hyphen was previously not allowed but is allowed now before I and reviewers were unaware.

It is being fixed in PR #5686

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