move virtual-merge method out of dor-services

sul-dlss-deprecated / dor-services

Common services, models, and utility classes used by the Stanford Digital Repository (DOR == Digital Object Registry)

Other

8 stars 4 forks source link

move virtual-merge method out of dor-services #652

Closed ndushay closed 4 years ago

ndushay commented 4 years ago

~~Currently virtual-merge is a script in dor-utils; we intend to migrate it to argo (see https://github.com/sul-dlss/dor-utils/issues/66).~~

possibly blocked by sul-dlss/argo#1458 ???? See comment below.

code for add_virtual_resource method should be moved to bulk activejob location in argo OR to ? dor-services-app ?
- from in content_metadata_ds (https://github.com/sul-dlss/dor-services/blob/v6.8.0/lib/dor/datastreams/content_metadata_ds.rb#L161-L184
- the virtual-merge is the only consumer of this code.
don't forget to migrate the test code.

ndushay commented 4 years ago

@jcoyne I am assigning this to you, as recent PRs in dor-services-app and dor-services-client imply I am maybe not even asking the right question with this ticket, and you already have work in progress to "do the appropriate thing" re: where this logic should live.

My main concern when writing this ticket was "if this code is used in one place ... then should it live where it is used?" Like models and concerns formerly in dor-services.

Perhaps you can just drop an explanation here in the ticket and then close it.

jcoyne commented 4 years ago

@ndushay I agree that this is a good proposal for a refactor. My only qualms is that it doesn't need to be done right now and it doesn't need to be done by me. I'd prefer we get the virtual-merge API working in a nominal way, and then refactor to make it clean.

mjgiarlo commented 4 years ago

@jcoyne :speech_balloon:

@ndushay I agree that this is a good proposal for a refactor. My only qualms is that it doesn't need to be done right now and it doesn't need to be done by me. I'd prefer we get the virtual-merge API working in a nominal way, and then refactor to make it clean.

This sounds sensible to me, and in my judgment we are generally good about making time later on to handle small refactorings such as this, particularly when it reduces duplication and confusion. (In this case, "later on" could be in a day or two, and anyone on the team could pick that ticket up as long as it's prioritized.)

jcoyne commented 4 years ago

I don't agree that ContentMetadataDS#add_virtual_resource should move out of dor-services. That method helps manipulate the datastream, and I think it makes sense for it to be a part of the model. Eventually the whole datastream code shouldn't be depended on by anything but dor-services-app.

ndushay commented 4 years ago

I think we should have a realtime discussion to bring the team on board with your vision of what should remain in dor-services and what should move to dor-services-app or elsewhere. I'll put it on storytime agenda (or it can be a post-standup discussion). If i'm the only one confused, it can be a 1:1, of course.

mjgiarlo commented 4 years ago

I think we should have a realtime discussion to bring the team on board with your vision of what should remain in dor-services and what should move to dor-services-app or elsewhere. I'll put it on storytime agenda (or it can be a post-standup discussion). If i'm the only one confused, it can be a 1:1, of course.

I am in favor of this and wonder if a new ADR could/might be the output of such a meeting, so that we have the vision (and the consensus behind it) documented somewhere we might expect it.

ndushay commented 4 years ago

actually, this would be a better discussion for the bi-weekly infradev mtg. It feels like an important thing to document; an ADR could be a fine way to go.

ndushay commented 4 years ago

I'm closing this per infradev meeting today.