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

Fix invalid path char in GoLang packaging on Windows #6484

Merged
merged 6 commits into from Aug 5, 2019
Merged

Fix invalid path char in GoLang packaging on Windows #6484

merged 6 commits into from Aug 5, 2019

Conversation

lauw70
Copy link
Contributor

@lauw70 lauw70 commented Aug 1, 2019

fix for #6482

What did you implement:

Normalize paths before matching

Closes #6482

How did you implement it:

I wrapped the paths in path.normalize

How can we verify it:

Deploy a Golang binary (built & packaged on Windows with linux as target) to AWS lambda and try to execute it.

Todos:

None

Is this ready for review?: YES
Is it a breaking change?: NO

(This is my first contribution on Github ever, feedback is much appreciated)

@exoego exoego changed the title Lauw70 patch for 6482 Fix invalid path char in GoLang packaging on Windows Aug 1, 2019
@pmuens pmuens self-assigned this Aug 2, 2019
@pmuens pmuens added this to the 1.50.0 milestone Aug 2, 2019
Copy link
Contributor

@pmuens pmuens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @lauw70 thanks a lot for working on this 👍

Could you add a regression test for this so that we can be sure that this won't happen again? Let us know if you need any help here!

-  added normalized path expectations in tests
@lauw70
Copy link
Contributor Author

lauw70 commented Aug 2, 2019

@pmuens I am awfully ashamed to admit that this will be my first time ever to write a regression test.

The problem is that the tests aren't even run when executed on a non-windows system.
I have no clue how I would change the tests to do so.

I did change the tests to account for this issue, but they only will run on Windows

@lauw70
Copy link
Contributor Author

lauw70 commented Aug 2, 2019

The problem is that the tests aren't even run when executed on a non-windows system.

Never mind, just saw that the tests are also run on Windows

Copy link
Contributor

@pmuens pmuens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @lauw70 thanks for updating this PR 👍

Glad to hear that you got the regression test up- and running. I just updated it slightly so that it's isolated. I'd say it's LGTM :shipit:

@pmuens pmuens merged commit 0296372 into serverless:master Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Path error when deploying a go runtime from Windows
2 participants