Skip to content

Add support for whitelisting environment variables to pass through (closes #76) #77

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 2 commits into from
Closed

Add support for whitelisting environment variables to pass through (closes #76) #77

wants to merge 2 commits into from

Conversation

dflemstr
Copy link
Contributor

This adds a fairly straight-forward section to Cross.toml like so:

[build.env]
whitelist_all = true
# ...or...
whitelist = ["PATH", "RVM_VERSION", "LD_LIBRARY_PATH"]

@dflemstr dflemstr changed the title Add support for whitelisting environment variables to pass through (fixes #76) Add support for whitelisting environment variables to pass through (closes #76) Mar 11, 2017
@japaric
Copy link
Contributor

japaric commented Apr 2, 2017

Thank you for the PR, @dflemstr, and sorry for taking so long to review.

The implementation looks good to me but I'm a bit concerned about whitelist_all as that makes more likely to break the operation of Cross by overriding one of the env variables its docker container internally uses. Ideally, I'd like Cross to warn you if you were to override one of such internal variables but that would make the implementation of this feature way more complicated.

So, how about this: let's implement just whitelist and document this feature on the README noting that Cross requires some env variables to be set to certain values, perhaps also list the common ones in the README, to work correctly and that you should think twice about overriding those.

What do you think?

@kamalmarhubi
Copy link
Contributor

I would like this to enable passing TravisCI env vars into the build. Let me know if I can help nudge this along!

@kamalmarhubi kamalmarhubi mentioned this pull request Apr 18, 2017
Merged
kamalmarhubi added a commit to kamalmarhubi/cross that referenced this pull request Apr 18, 2017
@kamalmarhubi
Remove whitelist_all
8aa3aec 
This addresses comments on cross-rs#77.
@kamalmarhubi kamalmarhubi mentioned this pull request Apr 18, 2017
Merged
kamalmarhubi added a commit to kamalmarhubi/cross that referenced this pull request Jun 6, 2017
@kamalmarhubi
Remove whitelist_all
81c74ab 
This addresses comments on cross-rs#77.
kamalmarhubi added a commit to kamalmarhubi/cross that referenced this pull request Jun 10, 2017
@kamalmarhubi
Remove whitelist_all
b2b5a09 
This addresses comments on cross-rs#77.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dflemstr @japaric @kamalmarhubi