Skip to content
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

Remove Hyrax::WorkingDirectory module #5693

Open
conorom opened this issue Jun 14, 2022 · 1 comment
Open

Remove Hyrax::WorkingDirectory module #5693

conorom opened this issue Jun 14, 2022 · 1 comment

Comments

@conorom
Copy link
Contributor

conorom commented Jun 14, 2022

Descriptive summary

While working on #5676, I was examining Hyrax::WorkingDirectory. This is only used when CharacterizeJob or CreateDerivativesJob are performed with no filepath parameter sent along.

I found that find_or_retrieve() is not compatible with versions when multiple versions have the same name. I removed the method (and its uses) from the code in PR #5694, instead opting to always pull a fresh repo copy if no filepath is provided to these jobs.

However the whole module was marked for removal in a TODO comment on this PR.

It was also marked as deprecated.

This seems kind of logical to me. I presume JobIoWrapper should take over the required action of getting the file from the repo if a job needs it?

When this is being done, consider whether the working_path configuration setting is needed at all.
I'm guessing it is. Plonking the repo-file into the Hyrax::UploadedFile-id-based directory (as used by CarrierWave when a file is uploaded through the UI) is probably a bad idea.

I'm not sure that these NOID-based paths used by repo files being used by jobs should end up under tmp/uploads by default, though.
They're not really uploads at that point, right? They just got pulled out of the repo.

OK that (and maybe this ticket in general) is a bit of a nitpick.
At a minimum the deprecation should be removed and the class documented if it's sticking around.

Related work

#1301

@no-reply
Copy link
Contributor

unfortunately, though Hyrax::WorkingDirectory is marked as deprecated, it's also in active use by CreateDerivativesJob.

in the meanwhile, we're also looking at deprecating JobIoWrapper as valkyrie work continues. the preferred usage for new code is to use config.derivatives_storage_adapter. this opens Hyrax to using S3 (and other storage strategies preferred by the application) rather than having hard coded internal logic for storing to paths on disk.

i'd been avoiding touching this code as well as JobIoWrapper and CreateDerivativesJob to avoid unneeded churn while that work moves forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants