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

Support for Environment variables in BUILD files. #12797

Closed
xlevus opened this issue Sep 8, 2021 · 7 comments · Fixed by #17652
Closed

Support for Environment variables in BUILD files. #12797

xlevus opened this issue Sep 8, 2021 · 7 comments · Fixed by #17652

Comments

@xlevus
Copy link
Contributor

xlevus commented Sep 8, 2021

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.

@Eric-Arellano
Copy link
Contributor

Thanks for the suggestion! I like this - we want to reduce the need for writing custom plugins when possible. This also semi relates to #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"))

@stuhood
Copy link
Member

stuhood commented Sep 8, 2021

Likely also related to #7022.

@xlevus
Copy link
Contributor Author

xlevus commented Sep 8, 2021

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?

@Eric-Arellano
Copy link
Contributor

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.

@stuhood
Copy link
Member

stuhood commented Sep 8, 2021

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).

xlevus added a commit to xlevus/pants that referenced this issue Sep 9, 2021
Allows rules/targets to easily support values that can be pulled from
the environment.

Alternative solution to pantsbuild#12797.
xlevus added a commit to xlevus/pants that referenced this issue Sep 27, 2021
Allows rules/targets to easily support values that can be pulled from
the environment.

Alternative solution to pantsbuild#12797.
kaos pushed a commit to kaos/pants that referenced this issue Sep 30, 2021
Allows rules/targets to easily support values that can be pulled from
the environment.

Alternative solution to pantsbuild#12797.
@da-tubi
Copy link

da-tubi commented Aug 3, 2022

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

@Eric-Arellano
Copy link
Contributor

With #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants