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 scoped npm packages in ${file()} variables #5312

Merged
merged 2 commits into from Jan 7, 2019

Conversation

Projects
None yet
4 participants
@Limess
Copy link
Contributor

Limess commented Sep 19, 2018

What did you implement:

Closes #4494 by extending the changes in #4528.

How did you implement it:

Changed the file name/path matching regex to a blacklist.
It now allows any valid characters but the excluded ones and should also work with Windows path separators if needed.

Additionally absolute paths are now allowed.

Added @ to the allowed variable syntax so file paths with @ are resolved.

I added a unit test to verify this case, which uses both changes, I can't see any related integration tests testing any variable resolution so have not added any.

How can we verify it:

Use any filename with ${file(./@scope/file)}.

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

Handle scoped packages in ${file()} variables
Currently @ is not a valid variable character, so it doesn't get parsed
as one.
When it does get parsed, @ is not considered valid as part of a file path
- we add a blacklist as per #4528 to
handle this case.

@Limess Limess changed the title Handle scoped packages in ${file()} variables Handle scoped npm packages in ${file()} variables Sep 19, 2018

@dschep

dschep approved these changes Jan 7, 2019

@dschep dschep merged commit 0896f31 into serverless:master Jan 7, 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.4%) to 91.482%
Details

This was referenced Jan 9, 2019

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

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