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
Get Deposit application running against f4 #538
Conversation
…po4 clients. Disabled references to ACL service for the time being. Resolved some dependency issues with commons-lang. Updated to java 8 in the places missed previously. Redis changed to use local server temporarily while the VM doesn't have an instance.
…gic to PIDs class to handle reserved PIDs, such as the content root collections object
… Updated port for fcrepo4 server in integration tests
…lude any models, and added unit test for it
…sses can override. Premis event object overrides isUnmodified
…eval of staging location for files to be ingested. Fixed retrieval of manifest locations during deposit record ingest
…d local test config
@@ -54,8 +49,8 @@ | |||
@Autowired | |||
private Dataset dataset; | |||
|
|||
@Autowired | |||
private FedoraAccessControlService accessControlService; | |||
//@Autowired |
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 should have put a comment in here, but this stuff is basically commented out as a reminder until we have access control worked out
URI stagingUri = new URI(stagingPath); | ||
file = new File(stagingUri); | ||
} catch (URISyntaxException e) { | ||
failJob(e, "Unable to staging URI: {0}", stagingPath); |
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.
"Unable to resolve staging URI"
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package edu.unc.lib.deposit.work; |
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.
Is this whole file being replaced by Camel messaging?
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.
Yes, most likely, and there wasn't anything to submit requests for indexing to in the fcrepo4 branch at this point
import edu.unc.lib.dl.rdf.Cdr; | ||
import edu.unc.lib.dl.util.URIUtil; | ||
|
||
public class RepositoryInitializerIT extends AbstractFedoraIT { |
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.
What does the IT stand for?
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.
"Integration Test"
@@ -15,7 +15,7 @@ | |||
# | |||
|
|||
# Root logger set to DEBUG and send to console for testing |
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.
This comment seems inaccurate. The logger is set to INFO, not DEBUG, no?
|
||
events.add(event); | ||
} | ||
|
||
log.debug("Retrieved {} events from file log for object {}", |
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.
Do these {} need an index inside them, like {0}?
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.
no, confusingly it works differently for log4j loggers versus the java string formatting method, which is used by the failJob method.
|
||
@Override | ||
public ContentContainerObject addMember(ContentObject member) throws ObjectTypeMismatchException { | ||
// TODO should only allow admin units, but allow folders for the moment |
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.
Is there a JIRA ticket for this, @bbpennel ? What do you think, in general, about us creating a new ticket for each TODO (or related set thereof), and putting a link to the ticket in the TODO comment, as well as a link to the TODO locus in the ticket? I think it could help us track this work better and keep pending enhancements more visible. The code itself would then provide some context on what the ticket is about. @lfarrell , do you have an opinion here, too?
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.
Good idea. Since our jira is internal, i feel more inclined to link from the ticket to the TODO.
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.
Another benefit is that if we have to shuffle tickets around we won't have to change the TODOs. One negative is that it is slightly weird to link to code on an unmerged branch that will go away, I'm not 100% sure if the link will stay live
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.
You raise some good points. Let's continue the discussion offline, maybe at our next sprint planning :-)
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.
sure, i've added a comment referencing this todo to the AdminUnit ticket
@@ -38,4 +38,13 @@ public int compareTo(PremisEventObject o) { | |||
return 0; | |||
} | |||
} | |||
|
|||
/** | |||
* Override to assume that the remote version will not change after creation |
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 find this comment kind of confusing. Maybe break into two sentences?
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.
done
|
||
// Don't initialize the object if it is already present. | ||
if (objectExists(contentRootUri)) { | ||
return contentUri; |
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.
Should this return contentRootUri?
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.
yup, it totally should
} | ||
|
||
/** | ||
* Show that additional initialization calls after the first do no cause |
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.
...not cause...
5c43701
to
0aac666
Compare
Gets the deposit pipeline functional to the point of being able to deposit a bag and its deposit record.