- 
                Notifications
    
You must be signed in to change notification settings  - Fork 158
 
Initialize directory structure with project #4750
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?
Conversation
        
          
                apps/dashboard/app/models/project.rb
              
                Outdated
          
        
      | Workflow.workflow_dir(project_dataroot) | ||
| JobLogger::JobLoggerHelper.log_file(project_dataroot) | 
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 not in love with the fact that you're calling these expressly for the side affects and not the return value.
The log_file especially should fix itself whenever upsert_job! or similar is called.
Can't speak to the workflow_dir but it seems like it should follow the same pattern of mkpath unless <it> exist?
There's got to be a better way here.
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.
It seems like both of these functions serve the dual role of accessing the correct path and making sure it exists (creating it if necessary)
ondemand/apps/dashboard/app/models/workflow.rb
Lines 7 to 11 in edb1d72
| def workflow_dir(project_dir) | |
| dir = Pathname.new("#{project_dir}/.ondemand/workflows") | |
| FileUtils.mkdir_p(dir) unless dir.exist? | |
| dir | |
| end | 
ondemand/apps/dashboard/app/models/concerns/job_logger.rb
Lines 51 to 55 in edb1d72
| def log_file(directory) | |
| Pathname.new("#{directory}/.ondemand/job_log.yml").tap do |path| | |
| FileUtils.touch(path.to_s) unless path.exist? | |
| end | |
| end | 
and both include the check for existence internally. The real issue is that if these files don't exist when the project is created, then someone else might open the project and lack the permissions to create the files. One other approach could be redirecting you to ProjectController#show after clicking save instead of ProjectsController#index, which would force the creator to be the first one to open it (and both of these methods are called during the show action).
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.
My guess is that the way it's written now just doesn't jive with shareable projects.
Seems to me that when the owner creates the project - we just initialize all required directories & files. The way this was originally written (likely by me 😆) doesn't account for/work well for this use case so we likely should refactor it to have better/cleaner initialization.
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.
Yeah I think that was what I was trying to achieve with these changes. Those methods seemed to be the source of truth on the paths, so I didn't want to define them again in a separate place. Though with these changes the project creation initializes these paths, what other refactoring did you have in mind? Or do you mean moving those methods to the Project class?
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.
We talked about this in person - but to keep the reference here I mean refactoring so that these methods don't have side affects. That is they can return a Pathname as they always have, but they shouldn't also create the path if it doesn't exist.
| 
           The refactor has been done, but there are a few places in the tests where we need to expect the changes in behavior  | 
    
Contributes to #4121. I ran into an error with the collaborative projects where a project can be created by one user, imported by another, but not opened by the non-owning user. The cause was that some directories under
.ondemand/were only created upon the first use, which was the call toProjectsController#show. So if the owner creates a project but never opens it, these directories are not created, and when the other user opens it, they do not have the permission to create them.This solves the issue by ensuring that all the necessary directories are created along with the project, so that we have an absolute guarantee that they are always created by the project owner. This allows a non-owning collaborator to be the first one to open a project without errors.
There is also the possibility that I have configured the project root incorrectly. The collaborating user currently has read and execute access to the project root, but not write access. As far as I understand this is what we expect shared projects to be created with, but open to correction if I am mistaken