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

handle layers paths with trailing slash and leading ./ or just . #5656

Merged
merged 2 commits into from Jan 29, 2019

Conversation

Projects
4 participants
@dschep
Copy link
Member

dschep commented Jan 7, 2019

What did you implement:

handle layers paths with trailing slash
closes #5646

How did you implement it:

use path.resolve on the layer path before using it as a prefix

How can we verify it:

create a service with a layer with a path ending in /

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

@pmuens
Copy link
Member

pmuens left a comment

This looks good @dschep 🤘

Could you add some unit tests? After that it should be GTM :shipit:!

@pmuens pmuens added this to In progress in Serverless via automation Jan 16, 2019

@pmuens pmuens self-assigned this Jan 16, 2019

@pmuens pmuens moved this from In progress to Needs review in Serverless Jan 16, 2019

@khalilgharbaoui

This comment has been minimized.

Copy link

khalilgharbaoui commented Jan 27, 2019

There is more going on than just a bug with railing slashes check:
#5646 (comment)

@dschep dschep force-pushed the sls-5646 branch from e371c59 to 3093c83 Jan 28, 2019

@dschep dschep changed the title handle layers paths with trailing slash handle layers paths with trailing slash and leading ./ or just . Jan 28, 2019

@dschep

This comment has been minimized.

Copy link
Member Author

dschep commented Jan 28, 2019

@khalilgharbaoui, yup. I've noticed, just slow moving on this PR cause I've had other things on my plate. It's updated now and should handle that case too. Do you mind testing it? npm i -g serverless/serverless/#sls-5646

@dschep

This comment has been minimized.

Copy link
Member Author

dschep commented Jan 28, 2019

@pmuens, reworked the implementation and added tests for the new way of fixing this

@pmuens pmuens added pr/in-review and removed pr/in-progress labels Jan 29, 2019

Serverless automation moved this from Needs review to Reviewer approved Jan 29, 2019

@pmuens

pmuens approved these changes Jan 29, 2019

Copy link
Member

pmuens left a comment

Thanks for updating @dschep 👍

Can conform that this works on my machine! LGTM :shipit:

@pmuens pmuens merged commit 1352d9b into master Jan 29, 2019

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 Jan 29, 2019

@pmuens pmuens deleted the sls-5646 branch Jan 29, 2019

@khalilgharbaoui

This comment has been minimized.

Copy link

khalilgharbaoui commented Jan 29, 2019

Working perfect now!

@pmuens pmuens added this to the 1.36.4 milestone Feb 5, 2019

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