-
Notifications
You must be signed in to change notification settings - Fork 109
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
check owner and group executable bits #727
Conversation
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.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks
a discussion (no related file):
nit: Please correct the paragraph part in the pull request to be grammatically correct.
a discussion (no related file):
Need a test
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.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks
a discussion (no related file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: Please correct the paragraph part in the pull request to be grammatically correct.
Haha nice catch! Just fixed it
a discussion (no related file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
Need a test
Got it, I'll work on that next
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.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks
a discussion (no related file):
Previously, zbirenbaum (Zach Birenbaum) wrote…
Haha nice catch! Just fixed it
Nice, but please remove the question now. This will become your commit message.
18c20c9
to
78de0a7
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.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks
a discussion (no related file):
Previously, zbirenbaum (Zach Birenbaum) wrote…
Got it, I'll work on that next
Test has been added and I confirmed that it fails without the change
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.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks
-- commits
line 5 at r2:
commits should be squashed
78de0a7
to
f98eaf3
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.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: pre-commit-checks
-- commits
line 4 at r3:
nit: should be in form:
Short tile without trailing period
Short paragraph of what changed.
[fixes|closes|towards] #ticket_number
nativelink-worker/tests/running_actions_manager_test.rs
line 145 at r3 (raw file):
#[cfg(test)] mod running_actions_manager_tests { use nativelink_proto::build::bazel::remote::execution::v2::command::EnvironmentVariable;
nit: Put this at top of file with other imports.
nativelink-worker/tests/running_actions_manager_test.rs
line 2586 at r3 (raw file):
fs::create_dir_all(&root_work_directory).await?; let running_actions_manager = Arc::new(RunningActionsManagerImpl::new_with_callbacks(
nit: I believe in this test we can use RunningActionsManagerImpl::new
unless you need those callbacks.
nativelink-worker/tests/running_actions_manager_test.rs
line 2618 at r3 (raw file):
"sh".to_string(), "-c".to_string(), "echo whoami > ${OUTS} && chmod 740 ${OUTS}".to_string(),
nit:
"touch {FILE_1_NAME} && chmod 700 {FILE_1_NAME}".to_string()
And remove the environ.
nativelink-worker/tests/running_actions_manager_test.rs
line 2650 at r3 (raw file):
..Default::default() }), salt: SALT,
nit: Inline these. Or better yet use ..Default::default()
and remove them.
nativelink-worker/tests/running_actions_manager_test.rs
line 2659 at r3 (raw file):
}; // Ensure the file copied from worker to CAS is executable. assert!(action_result.output_files[0].is_executable);
nit: Add description of error as second argument.
f98eaf3
to
540dc61
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.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: pre-commit-checks
Previously, adam-singer (Adam Singer) wrote…
commits should be squashed
Done.
Changes the running action manager to check the owner and group executable permissions of artifacts instead of just others. Prevents files from losing executable status when being copied from the worker to the CAS. Fixes TraceMachina#675
540dc61
to
d01b519
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.
Reviewed 1 of 2 files at r5.
Reviewable status: 1 of 1 LGTMs obtained
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.
Dismissed @adam-singer from a discussion.
Reviewable status: complete! 1 of 1 LGTMs obtained
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.
Reviewed 1 of 2 files at r5, all commit messages.
Reviewable status: complete! 2 of 1 LGTMs obtained
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.
Reviewed 1 of 1 files at r1, 1 of 1 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: complete! 2 of 1 LGTMs obtained
Description
Changes the running action manager to check the owner and group executable permissions of artifacts instead of just others. Prevents files from losing executable status when being copied from the worker to the CAS.
Fixes #675
Type of change
How Has This Been Tested?
This includes a regression test for losing executable permission when copying from worker to CAS.
Checklist
bazel test //...
passes locallyThis change is