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

Websockets: Support more route characters #5865

Merged
merged 2 commits into from Feb 28, 2019
Merged

Websockets: Support more route characters #5865

merged 2 commits into from Feb 28, 2019

Conversation

@eahefnawy
Copy link
Member

eahefnawy commented Feb 25, 2019

What did you implement:

Closes #5858

How did you implement it:

Replace special characters with alphanumeric ones, as we've done with other logical IDs

How can we verify it:

functions:
  hello:
    handler: handler.hello
    events:
      - websocket: 'echo'
      - websocket: 'echo/echo'
      - websocket: 'echo-echo'
      - websocket: 'echo_echo'
      - websocket: 'echo.echo'

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

@eahefnawy eahefnawy added this to the 1.39.0 milestone Feb 25, 2019
@eahefnawy eahefnawy added this to In progress in Serverless via automation Feb 25, 2019
@eahefnawy eahefnawy requested a review from pmuens Feb 25, 2019
Copy link
Contributor

dschep left a comment

Do we not have code we can reuse to do this? And even if not, should this more closely match other places we do this? ie, Dash instead of D.

@eahefnawy

This comment has been minimized.

Copy link
Member Author

eahefnawy commented Feb 26, 2019

@dschep I used Dash at first, but thought it'd be better to use single/double characters like D to keep the logical ID short, especially if there are multiple dashes and slashes, which I think would be common for websockets specially

@eahefnawy eahefnawy self-assigned this Feb 26, 2019
Copy link
Member

pmuens left a comment

Looks good so far 👍

I agree with @dschep that it might be better if the Ids follow our general naming conventions we've used for other events. However I understanding the thinking here that there could be multiple DashDash replacements which would result in really long logical ids.

Nevertheless we might want to update the docs and add our Websocket logical id schema there.

@dschep

This comment has been minimized.

Copy link
Contributor

dschep commented Feb 26, 2019

unless there's a size limit to contend with, at this point I'd rather not have a new scheme.

@eahefnawy

This comment has been minimized.

Copy link
Member Author

eahefnawy commented Feb 27, 2019

@dschep I did some research and couldn't find any particular size limit on logicalIds, so I've pushed your requested changes. Thanks for the feedback 👍

Serverless automation moved this from In progress to Reviewer approved Feb 27, 2019
@dschep
dschep approved these changes Feb 27, 2019
@pmuens
pmuens approved these changes Feb 28, 2019
Copy link
Member

pmuens left a comment

LGTM :shipit:

Not sure if we want to add this to the docs. Feel free to merge if you think that we don't need docs for this...

@eahefnawy

This comment has been minimized.

Copy link
Member Author

eahefnawy commented Feb 28, 2019

@pmuens yep, no need. It is the expected behavior.

@eahefnawy eahefnawy merged commit 85701b4 into master Feb 28, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Serverless automation moved this from Reviewer approved to Done Feb 28, 2019
@eahefnawy eahefnawy deleted the ws-normalize-route branch Feb 28, 2019
@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented Feb 28, 2019

FWIW here's what I found today, so we should stick to the pattern of replacing - with Underscore, etc. in the future https://github.com/serverless/serverless/blob/master/docs/providers/aws/guide/resources.md#override-aws-cloudformation-resource

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Serverless
  
Done
3 participants
You can’t perform that action at this time.