Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+39 −8
Open

Initialize directory structure with project #4750

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1f55d01
initialize workflow directory with project
Bubballoo3 Oct 30, 2025
9e98fd8
initialize log directory
Bubballoo3 Oct 30, 2025
606dade
correct namespace
Bubballoo3 Oct 30, 2025
4159c19
expect empty logfile in test
Bubballoo3 Oct 30, 2025
721635a
actually fix test
Bubballoo3 Oct 30, 2025
d2a054a
refactor directory preparation
Bubballoo3 Oct 31, 2025
7460dd6
create necessary files in tests
Bubballoo3 Nov 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions apps/dashboard/app/models/project.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,8 @@ def update_attrs(attributes)
def make_dir
project_dataroot.mkpath unless project_dataroot.exist?
configuration_directory.mkpath unless configuration_directory.exist?
Workflow.workflow_dir(project_dataroot)
JobLogger::JobLoggerHelper.log_file(project_dataroot)
Copy link
Contributor

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.

Copy link
Contributor Author

@Bubballoo3 Bubballoo3 Oct 31, 2025 •
edited
Loading

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).

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

true
rescue StandardError => e
errors.add(:save, "Failed to make directory: #{e.message}")
Expand Down
2 changes: 1 addition & 1 deletion apps/dashboard/test/models/projects_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ class ProjectsTest < ActiveSupport::TestCase
assert Dir.glob(cache_json_path).present? &&
Dir.glob("#{project.directory}/.ondemand/launchers/*/cache.json").empty?
assert Dir.glob(job_log_path).present? &&
Dir.glob("#{project.directory}/.ondemand/*").exclude?("#{project.directory}/.ondemand/job_log.yml")
File.read("#{project.directory}/.ondemand/job_log.yml").empty?
end
end

Expand Down