feat: add patch_node_fs attribute to js_binary and js_run_binary for easy opt-out #281
Conversation
gregmagolan
force-pushed
the
opt_out_fs_patches
branch
5 times, most recently
from
4537e58 to
fe09ce8
Compare
gregmagolan
force-pushed
the
opt_out_fs_patches
branch
from
fe09ce8 to
0ef1115
Compare
gregmagolan
changed the title
gregmagolan
force-pushed
the
opt_out_fs_patches
branch
from
0ef1115 to
eb8cc7c
Compare
| @@ -168,6 +168,19 @@ _ATTRS = { | |||
| values = _LOG_LEVELS.keys(), | |||
| default = "error", | |||
| ), | |||
| "patch_node_fs": attr.bool( | |||
There was a problem hiding this comment.
Should we try to put the word "sandbox" in the name since "patch" normally implies a bug-fix?
There was a problem hiding this comment.
sandbox wouldn't be accurate either since the guards prevent leaving execroot, runfiles and the sandbox. I create a pretty verbose docstring on this one. most users will never need to touch it but it does leave an easy opt-out if needed.
There was a problem hiding this comment.
node_fs_guards perhaps but I like "patch" as it is technically what we are doing by changing the behavior of the node fs code
There was a problem hiding this comment.
I guess I'd worry that fs will become overloaded and will mean patching a lot more then the node "fs" library like the name implies 🤷
There was a problem hiding this comment.
Hopefully we won't be patching much more in node so we won't to worry about it :)
| @@ -168,6 +168,19 @@ _ATTRS = { | |||
| values = _LOG_LEVELS.keys(), | |||
| default = "error", | |||
| ), | |||
| "patch_node_fs": attr.bool( | |||
There was a problem hiding this comment.
The child process patch will also have a flag? patch_node_child_process?
There was a problem hiding this comment.
I think the child process fs patches should just be implied if this is true since they are the same fs patches applied to spawn processes; just more a more robust implementation that covers the child spawn case when we get to it for 1.1
There was a problem hiding this comment.
Ah yeah, you're correct... it's the same patches but just properly applied to sub-processes as well
No description provided.