-
Notifications
You must be signed in to change notification settings - Fork 162
Toggle group ownership on projects #4792
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
base: master
Are you sure you want to change the base?
Changes from all commits
7062548
9bca1b2
934a2fc
0d2b0fe
5a31c92
28fa47f
463fc20
62aa0b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,7 +111,7 @@ def importable_directories | |
| end | ||
| end | ||
|
|
||
| attr_reader :id, :name, :description, :icon, :directory, :template, :files | ||
| attr_reader :id, :name, :description, :icon, :directory, :template, :files, :group_owner | ||
|
|
||
| validates :name, presence: { message: :required }, on: [:create, :update] | ||
| validates :id, :directory, :icon, presence: { message: :required }, on: [:update] | ||
|
|
@@ -128,6 +128,7 @@ def initialize(attributes = {}) | |
| @directory = attributes[:directory] | ||
| @directory = File.expand_path(@directory) unless @directory.blank? | ||
| @template = attributes[:template] | ||
| @group_owner = attributes[:group_owner] || get_group_owner | ||
|
|
||
| return if new_record? | ||
|
|
||
|
|
@@ -203,6 +204,29 @@ def remove_from_lookup | |
| false | ||
| end | ||
|
|
||
| def private? | ||
| project_dataroot.to_s.start_with?(CurrentUser.home) | ||
| end | ||
|
|
||
| def get_group_owner | ||
| if project_dataroot != Project.dataroot && project_dataroot.grpowned? | ||
| Etc.getgrgid(project_dataroot.stat.gid).name | ||
| else | ||
| nil | ||
| end | ||
| end | ||
|
|
||
| def set_group_owner | ||
| return true if private? || @group_owner == get_group_owner | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it's an |
||
| begin | ||
| group_gid = @group_owner.nil? ? nil : Etc.getgrnam(@group_owner).gid | ||
| FileUtils.chown(nil, group_gid, project_dataroot) | ||
| rescue StandardError => e | ||
| errors.add(:update, "Unable to set group ownership with error #{e.class}:#{e.message}") | ||
| false | ||
| end | ||
| end | ||
|
|
||
| def editable? | ||
| File.writable?(manifest_path) | ||
| end | ||
|
|
@@ -305,6 +329,7 @@ def update_attrs(attributes) | |
|
|
||
| def make_dir | ||
| project_dataroot.mkpath unless project_dataroot.exist? | ||
| set_group_owner | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we actually move this to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Errr.... actually it is fine here. We should maybe just pass 750 to I say that it's fine here, because we'll also need to setgid bit for shared projects and we should do that before we make the other directories so that they're initialized under the correct group.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It certainly would logically, but as soon as project_dataroot has files, we end up in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I wonder if this prompts a reorganization to the setup steps here. Maybe we have a method |
||
| configuration_directory.mkpath unless configuration_directory.exist? | ||
| workflow_directory = Workflow.workflow_dir(project_dataroot) | ||
| workflow_directory.mkpath unless workflow_directory.exist? | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -175,6 +175,7 @@ en: | |
| jobs_project_directory_error: Project directory path is not set for this workflow | ||
| jobs_project_directory_placeholder: Project directory absolute path | ||
| jobs_project_generic_error: 'There was an error processing your request: %{error}' | ||
| jobs_project_group_owner: Group owner | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm struggling a bit with the word A quick google search indicates it's just
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So maybe a label like 'Create as group:' would be more accurate? Or maybe 'Create with group:' would be better. Or are you saying just 'group' by itself is clear enough?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may be overthinking it. Maybe it's fine.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No I think it is an important thing to get right. We basically have to find a compromise between people with and without a working knowledge of linux, and be attentive to what will give the best understanding to everyone. 'Group owner' is almost bad both ways, as it is both confusing to someone who knows that groups cannot be an 'owner' of a file, and someone without that knowledge could mistakenly think they are doing just that, and voiding their personal ownership and control of the project. Especially without any auto-detection helping with the choice, it is important that people know that this is a necessary step for sharing their project.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a little
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I like that. I think I'll probably go with 'Group' to be as minimal and accurate as possible, and then explain in the help text like |
||
| jobs_project_invalid_configuration_clusters: An HPC cluster is required. Contact your administrator to add one to the system. | ||
| jobs_project_invalid_configuration_scripts: An executable script is required for your project. Upload a script using the file application. | ||
| jobs_project_job_deleted: Successfully deleted job %{job_id} | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.