pants icon indicating copy to clipboard operation
pants copied to clipboard

Support for Environment variables in BUILD files.

Open xlevus opened this issue 2 years ago • 7 comments

Is your feature request related to a problem? Please describe. In two cases I've written a plugin that needs to access environment variables, and will need to write a third. Two of which, are documented cases for writing plugins.

  • Plugin to take the $VERSION variable and insert it into setup_py()
  • Username/Password for a PyPi repository
  • (to come) take $VERSION and use it as the docker_image(version= .

It would be much easier if by default these fields could take environment variables by default.

Describe the solution you'd like

An Env 'function' in BUILD files to denote the value of that field is to be taken from an environment variable.

python_distribution(
  ...
  setup_py(
    version=Env('VERSION'),
  )
)

docker_image(
  ...
  version=Env('VERSION'),
)

pypi_repository(
  password=Env('PYPI_PASSWORD')
)

Other Considerations

  • If the one of the suggested uses for this is secrets, care must be taken to ensure any caching does not inadvertently store the values.
  • What should happen when the variable is unset or 'falsly' in the environment?

Describe alternatives you've considered

Alternatively, these fields could be made to support environment variable interpolation by default. e.g. docker_image(version='${VERSION}'). While this would also work, it would require Pants developers to second-guess/dictate where environment variables might be used and increase the complexity.

xlevus avatar Sep 08 '21 22:09 xlevus

Thanks for the suggestion! I like this - we want to reduce the need for writing custom plugins when possible. This also semi relates to https://github.com/pantsbuild/pants/issues/10399.

FYI the reason we ban imports of os.environ in BUILD files is it would break caching/memoization - your Env suggestion meanwhile would allow us to get that caching right.

--

Another consideration is how to handle data types. Would we expect the user to do this?

python_tests(timeout=int(Env("MY_TIMEOUT"))

That's legal because BUILD files are Python and int() is builtin.

An alternative is for the Target API to start accepting strings for non-strong fields and trying to convert. We only need to update the core definitions of IntField, FloatField, etc for all consumers to do the right thing. This means this snippet would work, where Env is giving back a str:

python_tests(timeout=Env("MY_TIMEOUT"))

Eric-Arellano avatar Sep 08 '21 22:09 Eric-Arellano

Likely also related to #7022.

stuhood avatar Sep 08 '21 22:09 stuhood

Another consideration is how to handle data types. Would we expect the user to do this?

python_tests(timeout=int(Env("MY_TIMEOUT"))

That's legal because BUILD files are Python and int() is builtin.

An alternative is for the Target API to start accepting strings for non-strong fields and trying to convert. We only need to update the core definitions of IntField, FloatField, etc for all consumers to do the right thing. This means this snippet would work, where Env is giving back a str:

python_tests(timeout=Env("MY_TIMEOUT"))

From a users perspective, I would think having the field responsible for string-parsing would be the better option. Why should I, as a user, be responsible for making sure my fields are cast to the right type?

From a developers perspective, while there are currently Int's and Floats as core fields, what about Lists-of-ints, and maps-of-floats and other more complex data structures?

xlevus avatar Sep 08 '21 22:09 xlevus

Likely also related to #7022.

Sort of. If "target generation" lands, that will mean that you can write much more powerful macros that have access to the Rules API. Rather than using our custom plugin hooks for things like setup_py(), you can write a more generic macro to generate a python_distribution target.

But, that still would mean writing a plugin. Iiuc this issue is about allowing you to access env vars without needing to write a plugin. Much lower barrier to entry and maintenance burden for users.

--

From a users perspective, I would think having the field responsible for string-parsing would be the better option.

Cool, agreed!

While there are currently Int's and Floats as core fields, what about Lists-of-ints, and maps-of-floats and other more complex data structures?

That should be doable :) For example, with a list of ints, it's probably too hard to implement parsing of "[1, 2, 3]" - but we could support [1, ENV("FOO"), 3] fairly easily.

Eric-Arellano avatar Sep 08 '21 23:09 Eric-Arellano

Likely also related to #7022.

Sort of. If "target generation" lands, that will mean that you can write much more powerful macros that have access to the Rules API. Rather than using our custom plugin hooks for things like setup_py(), you can write a more generic macro to generate a python_distribution target.

I meant more that until #7022, there isn't a way to create either a macro or object that safely consumes the Env in a way that will be properly invalidated (without hacking deeply in BUILD parsing to special case this).

stuhood avatar Sep 08 '21 23:09 stuhood

With #7022 , how to write a macro to set by the env variable?

da-tubi avatar Aug 03 '22 03:08 da-tubi

With https://github.com/pantsbuild/pants/issues/7022 , how to write a macro to set by the env variable?

Hey @da-tubi, the result of #7022 was the new Target Generator API added in Pants 2.8, the section "File-level metadata with overrides" of https://blog.pantsbuild.org/introducing-pants-2-8/.

The only way currently consume environment variables in a BUILD files is by writing plugins using the Rules API, like a target generator, or the setup_py() kwargs plugin hook.

Eric-Arellano avatar Aug 03 '22 15:08 Eric-Arellano