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

Adds envVars propagation client -> server #267

Merged

Conversation

Baccata
Copy link
Contributor

@Baccata Baccata commented Mar 29, 2018

Since Mill now executes in a long-lived JVM, the builds do not have a chance to use environment variables as inputs (ie System.getEnv returns the same result across several runs).

This PR allows to propagate the environment variables from the client all the way down to the context and makes them available as a Map[String, String] so that they can be used as inputs should the user choose to do so.

Example of usage :

def envVar = T.input { T.ctx().env.get("ENV_VAR") }

#257

  • Tests

  • Documentation

Since Mill now executes in a long-lived JVM, the builds do not have a
chance to use environment variables as inputs. This propagates the
environment variables from the client all the way down to the context
available to the tasks as a `Map[String, String]` so that they can be
used as inputs should the user choose to do so.

com-lihaoyi#257
@lihaoyi
Copy link
Member

lihaoyi commented Mar 30, 2018

@robby-phd @rockjam one of you want to help review this?

@rockjam
Copy link
Contributor

rockjam commented Mar 30, 2018

@lihaoyi will take a look

@rockjam rockjam self-requested a review March 30, 2018 07:31
@rockjam
Copy link
Contributor

rockjam commented Mar 31, 2018

@Baccata thank you, overall looks good.
Can we have a test at ClientServerTests that env variables are actually passed to server, and also add doc section about prefered way to get env variable in build? (on this page I guess http://www.lihaoyi.com/mill/page/configuring-mill.html)

@Baccata
Copy link
Contributor Author

Baccata commented Apr 2, 2018

Thanks for the feedback, I'll get on adding tests/doc asap

Baccata added 2 commits April 5, 2018 11:46
* Moved the `System.getenv` side effect to the end of the world
* Adds a test to make sure that the `Map[String, String]` gets
propagated correctly
* Adds a `Ctx.Env` trait for consistency

com-lihaoyi#257
Adds documentation about the Ctx.Env API to indicate that the user
should not use `System.getenv`

com-lihaoyi#257
@Baccata Baccata force-pushed the MILL-257-propagate-environment-variables branch from 7c7fc25 to 997760e Compare April 5, 2018 11:13
@Baccata
Copy link
Contributor Author

Baccata commented Apr 5, 2018

@rockjam, for consistency I added documentation there : https://github.com/lihaoyi/mill/pull/267/files#diff-521aff6dd9f50dbf1ce6d38d8940bdd1R191. Hope that's okay.

Also added a test

@rockjam
Copy link
Contributor

rockjam commented Apr 5, 2018

@Baccata great that you find a good place to put it in docs. I'll take a closer look at PR when I have time.

@lihaoyi lihaoyi force-pushed the master branch 2 times, most recently from 9ebd19b to 779e453 Compare April 6, 2018 22:55
Copy link
Contributor

@rockjam rockjam left a comment

Choose a reason for hiding this comment

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

LGTM

@rockjam
Copy link
Contributor

rockjam commented Apr 8, 2018

@Baccata There are some conflicts, can you take a look, will merge after that.

@lihaoyi lihaoyi merged commit 997760e into com-lihaoyi:master Apr 8, 2018
@rockjam
Copy link
Contributor

rockjam commented Apr 8, 2018

@lihaoyi thank you

@Baccata Baccata deleted the MILL-257-propagate-environment-variables branch April 9, 2018 07:14
@Baccata
Copy link
Contributor Author

Baccata commented Apr 9, 2018

@rockjam, sorry, was travelling all day yesterday, didn't see this until now. Thanks @lihaoyi !

@lefou lefou added this to the 0.2.0 milestone May 2, 2019
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.

4 participants