Skip to content

Add environment variables to docker run cmd #231

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add environment variables to docker run cmd #231

wants to merge 1 commit into from

Conversation

galindro
Copy link

@galindro galindro commented Sep 4, 2018 •
edited
Loading

Fixes #230

This was referenced Sep 4, 2018
Closed
Closed
@AndrewFarley
Copy link
Contributor

AndrewFarley commented Sep 4, 2018 •
edited
Loading

Haha... uhh @galindro you cloned "my" fork, and tried to MR it here. If you can, re-fork from this codebase...

git remote add upstream https://github.com/UnitedIncome/serverless-python-requirements.git
git reset --hard upstream/master

Then patch your changes, and force push so we get just your feature here.

And secondarily, I'd suggest making a test (look in test.bats) to prove/ensure this feature works, and continues to work as other changes occur. And finally, of course make sure to run the linting (see package.json) to ensure your code is linted properly or it won't pass CI. Thanks!

@galindro
Copy link
Author

galindro commented Sep 4, 2018 •
edited
Loading

I've forked your repo because I've did 1 enhancement + 1 bug fix in the same PR. The enhancement can be applied to current UnitedIncome/serverless-python-requirements master branch but the fix no. It depends from your fork.

Therefore, what can I do is:

  1. Update this PR with a single commit to add the docker environment feature
  2. Create a PR in your fork to fix the problem that I've mentioned with the command executed in docker run: replace && by ;. This way you can push a new version of it to your already open PR: Download and static caching as a feature #165

What do you think @AndrewFarley?

@galindro
Copy link
Author

galindro commented Sep 4, 2018

@AndrewFarley my time today is a little short... I've did the first proposal and I'm waiting by your answer for the second.

@AndrewFarley
Copy link
Contributor

If you don't have time just let it set for now, I'll get around to helping this get merged after my other PR gets merged.

@galindro
Copy link
Author

galindro commented Sep 4, 2018

Tks @AndrewFarley. I aprecciate your feedback.

@dschep
Copy link
Contributor

dschep commented Sep 13, 2018

Hey @galindro, I've merged @AndrewFarley's PR, you should be able to rebase on master now 😃

@galindro
Copy link
Author

Done @dschep . I took advantage to make the two changes aforementioned here

dschep
dschep reviewed Sep 13, 2018
View reviewed changes
lib/pip.js
@@ -210,7 +219,7 @@ function installRequirements(targetFolder, serverless, options) {
])
);
}
pipCmd = ['/bin/bash', '-c', '"' + commands.join(' && ') + '"'];
pipCmd = ['/bin/bash', '-c', '"' + commands.join(' ; ') + '"'];
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the motivation for changing this? Would you really want to keep running commands if one of them fails?

dschep added a commit that referenced this pull request Oct 23, 2018
@dschep
Pass env vars to docker
51456da 
Based on #231 & closes #267
This was referenced Oct 23, 2018
Closed
Merged
dschep added a commit that referenced this pull request Nov 8, 2018
@dschep
Pass env vars to docker (#271)
eabbd89 
* Add environment variables to docker run cmd

* Pass env vars to docker

Based on #231 & closes #267

* format

* fix docs

* Update README.md
bweigel pushed a commit to bweigel/serverless-python-requirements that referenced this pull request Nov 17, 2018
@dschep @bweigel
Pass env vars to docker (serverless#271)
abc9a10 
* Add environment variables to docker run cmd

* Pass env vars to docker

Based on serverless#231 & closes serverless#267

* format

* fix docs

* Update README.md
bweigel pushed a commit to bweigel/serverless-python-requirements that referenced this pull request Nov 17, 2018
@dschep
Pass env vars to docker (serverless#271)
6fadd07 
* Add environment variables to docker run cmd

* Pass env vars to docker

Based on serverless#231 & closes serverless#267

* format

* fix docs

* Update README.md
@miketheman
Copy link
Contributor

#Triage
This PR has a bit of history to it, and needs to be reconfirmed whether the original issue reported remains a problem.
@AndrewFarley you had some historical context on this?

@AndrewFarley
Copy link
Contributor

This indeed still looks useful, and it's quite a minor patch, could use a rebase and additions to the README, and perhaps a test.

I'll take this and get it prepared for merge @miketheman

@miketheman
Copy link
Contributor

This appears to have been resolved in #271. Closing!

@miketheman miketheman closed this Feb 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dschep dschep dschep left review comments

Assignees

@AndrewFarley AndrewFarley

Labels
Docker
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dockerizePip: "git clone failed with error code 128 in None" error
4 participants
@galindro @AndrewFarley @dschep @miketheman