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

Enable handler_name to be a relative path #1775

Closed
wants to merge 3 commits into from
Closed

Enable handler_name to be a relative path #1775

wants to merge 3 commits into from

Conversation

JakeChampion
Copy link

@JakeChampion JakeChampion commented Nov 19, 2019

Attempt to fix #1774

I agree to the contributor license agreement

@whummer
Copy link
Member

whummer commented Nov 19, 2019

Thanks for tackling this @JakeChampion . Do you think we could add a small unit test here: https://github.com/localstack/localstack/blob/master/tests/unit/test_lambda.py ? That would be great - thanks!

@whummer
Copy link
Member

whummer commented Nov 22, 2019

Any updates on this PR @JakeChampion ? Looks like the tests in CI are currently failing. Would be great if we could fix them to get this merged. Thanks!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.009%) to 51.926% when pulling 621e17b on JakeChampion:path-relative-handlers into bf76bdf on localstack:master.

@JakeChampion
Copy link
Author

@whummer tests passing now

@whummer
Copy link
Member

whummer commented Nov 23, 2019

Thanks for updating the PR @JakeChampion . I'd still feel a bit more comfortable if we could add a few unit tests, as this could have impact for a lot of people and could potentially break existing tests out there.

For example, what is the expected behavior if the handler name is ./test.file.handler - should it be function handler in ./test.file.js or //test/file.js?

Another example is the handler name .myfile.handler - this would currently map to function handler in file /myfile.js, but shouldn't it be .myfile.js?

I'm wondering if we should simply do this instead:

delimiter = '.'
if runtime.startswith(LAMBDA_RUNTIME_NODEJS):
    handler_name = handler_name.rpartition(delimiter)[0]
...
return '%s%s' % (handler_name, file_ext)

Thoughts?

@whummer
Copy link
Member

whummer commented Dec 7, 2019

@JakeChampion Any updates on this PR - do you think we could add a few unit tests to ensure the new logic covers any corner cases, and to avoid any regressions? Thanks

@JakeChampion
Copy link
Author

@whummer I don't think I can do that, I don't know Python nor how this code-base and it's tests work. I'm happy to leave the rest of the pull-request for someone from the core-team and/or community to take over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: handler paths which contain periods do not work
3 participants