https://pantsbuild.org/ logo
Join Slack
Powered by
hey folks was talking to <@UB2J9BQA0> about the en...
# development
w
hey folks was talking to @hundreds-father-404 about the env var proposall here: https://github.com/pantsbuild/pants/issues/10644, and while i have no issues with it, was wondering if there had been any consideration to preserve the entire original environment. This technically does not. violate hermeticity and can be quite useful for CI/CD environments that load multiple credentials or other things via env
w
any env var that was included would change the cache key of the running process
and so your various users might not hit shared remote caches
so it does affect hermeticity a bit.
w
sure. but affect != violate. right? definitely would trigger more cache misses
w
but that’s why that ticket suggests configuring which ones you want to include: it’s definitely useful, you just don’t want random stuff like this one in there:
Copy code
$ env | grep LaunchInstance
LaunchInstanceID=BFA89F91-2DC0-4950-BA60-5670917DE455
(…possibly literally random. i don’t know why that is in my env 😅)
w
sure. that makes sense. OTOH, we load CI/CD utils in jenkins from another repo controlled by another team, and then one from our team, so to 'add' something to our CI/CD environment would require a. change in possibly. three places
seems very. likely someone will forget. to tack on the option
definitely agree with the. risks involved, but wonder. if its a useful option. to have while defaulting to off
w
@wonderful-iron-54019: yes, in a “tree falling in the forest” situation, env vars might not affect the run. from a safety first perspective, we’ve tried to bias toward opting-in to including things rather than opting-in to exclude them… in both cases you might need to whack moles to include/exclude stuff… but one is a safer default maybe?
h
I wonder if this would be slightly more tolerable if it was an env allowlist? You could set 
--pex-extra-path=['FOO', 'BAR']
, and that would signal to read from 
os.env
, rather than having to set 
'FOO="${FOO}"
You could also explicitly set the env var if you want. (Either way, I think this is a better design than the proposal)
w
but also, consider that hermeticity might be even more important in CI environments, where you need more trust in the results, and are more likely to be running “everything that hasn’t changed since the last run” (and thus be more interested in hitting caches)
👍 1
yea, an allowlist would be a good idea. possibly even overridable per test.
(sorry, i think that i thought that that was already what the ticket was suggesting: should have bothered to read it)
h
I like the idea of specifying env vars a user would like a test to have access to via a command-line option
I'm not sure if it should be pex-specific
h
vs. applying to any subprocess ever run, right? We don’t have a solid approach right now for where options like that should live. I think you’re right that we need it to apply to more than Python, though. For example, the Protobuf subprocess should use the same env vars.
w
yeah concur with that, we have a few custom plugins that only marginally deal with python we'd want to have. this feature
h
maybe the scope of this option should be any subprocess pants spawns?
or is that too broad?
w
although we’ll certain need some infrastructure support that would allow rules to set these, the scope of the setting should definitely just be tests i think.
run
and 
repl
are in the foreground, and don’t need this setting at all… should just be able to use any env vars without a setting.
…oh. but the (original) usecase was during pex building? ie to pass to PEX/pip?
h
Ditto that 
--pex-executable-search-paths
might be the wrong place for where you control the $PATH env var. Consider if you are using a Protobuf plugin and that for some reason needs to discover something via the $PATH. I wonder if that option should apply to everywhere.
w
@hundreds-father-404: yea, that’s covered on the followup PATH ticket (https://github.com/pantsbuild/pants/issues/10526)
👍 1
h
oh. but the (original) usecase was during pex building? ie to pass to PEX/pip?
Yeah, the original motivating case was so that a test could consume the env var. Which might imply we want a 
--pytest-extra-env
option
👍 1
w
the test case is clearer, i think. unsure whether it should be “allow” vs “set this literal value” though.
h
Vs either or. You can use the shorthand, but can also set the literal value
👍 1
w
oh. dangit. another “i haven’t read the ticket”. sorry
h
I agree in general with trying to limit this to 
test
for now, as it’s easier to loosen things than to take away functionality. So long as that solves JP’s issue
w
yep. i like the syntax on the ticket.
is that the syntax that 
tox
uses?
h
Indeed. 
tox
is what inspired the allowlist shorthand. I think they have separate settings for literal vals vs. allowlist iirc, though
Open in Slack
PreviousNext
Powered by