-
Notifications
You must be signed in to change notification settings - Fork 543
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
fix: pass kwargs env
to both update and test targets
#2277
Conversation
Seems reasonable to me. If it's not too much a hassle, it'd be good to write an automated test for this to avoid future breakage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a CHANGELOG note to mentioned what was fixed?
Done! |
5d26d19
to
6c29c8a
Compare
|
||
py_binary( | ||
name = name + ".update", | ||
env = env, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised that we need the fake USERPROFILE
only for _test
and not for .update
too.
Why is that the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... X_test
runs as a Bazel action. X.update
is only expected to be invoked by the user directly, in a normal environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a possible source of confusion to have different behavior here, but I trust your (and @aignas's) judgement on this.
6c29c8a
to
f66e55a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Fixes #2270.