Conversation
tdonohue
left a comment
There was a problem hiding this comment.
@benbosman : I have a few early questions here. See below.
harvesting_metadata_configs.md
Outdated
| [Back to the list of all defined endpoints](endpoints.md) | ||
|
|
||
| ## Main Endpoint | ||
| **/api/config/harvesting_metadata_configs** |
There was a problem hiding this comment.
I think we need to better align these two endpoint names.
/api/config/harvesting_metadata_configs/api/core/collections/<:uuid>/harvestingSettings
I'd like to make these more similar. Currently one is camelcase, while the other uses underscores. One uses the term "Settings" while the other uses "Configs".
Maybe these should be called:
/api/config/harvestingMetadataConfig/api/core/collections/<:uuid>/harvestingConfigor just/api/core/collections/<:uuid>/harvesting
Other names are fine too, I just would like to see a more consistent name here.
There was a problem hiding this comment.
I agree with Tim, if we look at other /config/* endpoints, we will have:
- /api/config/submissiondefinitions
- /api/config/submissionsections
- /api/config/submissionforms
- /api/config/submissionuploads
I think it should be coherent with what was defined. And based on what is already defined, my suggestion goes to rename it:
from:
/api/config/harvesting_metadata_configs
to:
/api/config/harvestingmetadataconfigs
Note: initially I was wondering if the name make sense or should be under collection (like: collectionharvestingdefinitions), since this only applies to collections. But that name could be ambiguous.
Or perhaps if don't agree with the format, we should revisit the old ones if we plan to use a different format.
There was a problem hiding this comment.
Thinking on this a bit more (and based on @paulo-graca's notes), as the word /config is already in the path, I'd like to recommend we consider shortening these long path names to something like:
/api/config/harvestermetadata/api/core/collections/<:uuid>/harvester
I'm suggesting to use the term "harvester" as it's a noun (instead of "harvesting" which is an action verb). Plus it's the same term used in the backend configuration: https://wiki.duraspace.org/display/DSDOC6x/OAI#OAI-OAI-PMH/OAI-OREHarvester(Client)
I've also dropped "config" or "settings" from these paths, as none of our other configurable based endpoints append those terms...they are essentially implied.
There was a problem hiding this comment.
I've update the PR based on the latter proposal, assuming this is ok for everyone
| } | ||
| }, | ||
| "_embedded": { | ||
| "metadata_configs": { |
There was a problem hiding this comment.
Is there a reason for embedding all metadata_configs on a simple GET? It seems like those are also available on the other endpoint, so I'm wondering whether they need embedding here?
There was a problem hiding this comment.
Embedding is never required, but when the harvesting settings are requested, the list of potential metadata configs is typically relevant as well
Since it's only info from a config file, it's a very low-load detail to embed compared to the added value of reducing the amount of requests
There was a problem hiding this comment.
@benbosman : Ok, that's fine. One note here is that we likely should correct the name here from metadata_configs to harvestermetadata (as that's the new self link path)
paulo-graca
left a comment
There was a problem hiding this comment.
Thank you @benbosman !
Generally this PR it's ok by me. I just share some of Tim's questions/concerns, but I'm ok with whatever is the outcome for them.
tdonohue
left a comment
There was a problem hiding this comment.
👍 Looks good to me now. Thanks @benbosman
REST contract for viewing and configuring the OAI Harvesting settings of a collection.
This also includes an endpoint to retrieve a list of all configured supported harvesting formats.
This doesn't include starting the actual harvest action. It can be supported using #17