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

Adding -f and -i to requirements.txt options ignore list before sorting #238

Merged
merged 4 commits into from
Sep 14, 2018

Conversation

AndrewFarley
Copy link
Contributor

Fixes: #236 (again, properly)

Problem: requirements.txt options which must be in the proper order at the start of the file were being sorted. This properly skips sorting them and keeps them in order they were.

@AndrewFarley
Copy link
Contributor Author

@paddycarey I believe this will fix your requirements.txt problem...

rm -Rf node_modules/serverless-python-requirements
npm i github:andrewfarley/serverless-python-requirements#bug-fix-requirements-options

@paddycarey
Copy link

This seems to fix my problem, my private packages now install correctly, thanks!

Copy link
Contributor

@dschep dschep left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @AndrewFarley

@dschep dschep mentioned this pull request Sep 13, 2018
dschep added a commit that referenced this pull request Sep 14, 2018
 * Don't skip non-docker tests when docker isn't present
 * Don't hard code a static cache md5sum


@AndrewFarley, I noticed that tests were failing on #238 and got the same errors locally on `master`. Do you think it's reasonable to compute the md5sum this way? I don't like hard coding the hash. But, I understand that this makes the test verify less of the process. Best would probably be to externally replicate what `genrerateRequirementsFile` does to create a new file and md5sum that instead of `.serverless/requirements.txt`
@dschep
Copy link
Contributor

dschep commented Sep 14, 2018

Should -e maybe be added? re: #240

@AndrewFarley
Copy link
Contributor Author

AndrewFarley commented Sep 14, 2018

@dschep No, -e is a line prefix, not a whole file prefix. Any/every line can be prefixed with -e. I'd recommend that individual try this merge request however. Thanks for looking out though. I'll go look into his issue now see if I can replicate and debug /fix it

@dschep dschep merged commit b4ecc43 into serverless:master Sep 14, 2018
@AndrewFarley AndrewFarley deleted the bug-fix-requirements-options branch February 28, 2020 04:06
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.

3 participants