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

CADC-8776 update doi service int-test #9

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jburke-cadc
Copy link
Collaborator

  • int-tests no longer use the doiadmin user to run tests, the developer can use either a local VOSpace service they own or their home folder in a production VOSpace service.
  • individual CRUD int-tests replaced with a single DOI lifecycle int-test
  • intTest.properties file to set the doi service for testing and the VOSURI for the parent DOI folder in VOSpace.

@@ -0,0 +1,7 @@
# intTest.properties expects the following entries:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this file be in src/intTest directory instead?

The `doi-auth.pem` user is a member of this group, giving this user read/write access to a test DOI.
The `doi-noauth.pem` user is not a member of this group, giving this user read-only access to a test DOI.
- `doi-admin.pem` owns and has full permissions to a test DOI.
- `doi-auth.pem` has read-write permissions to a test DOI.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How? A group is required for that I assume.

DOI_PARENT_PATH = vosURI.getPath();
}

// s = props.getProperty("metadataFilePrefix");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleanup?

log.info("failed to load/read config", e);
}

}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this supposed to fail on ok=false?

throw new IllegalStateException(String.format("node %s must have inheritPermissions set to true", path));
}
// check inheritPermissions is true (does inheritPermissions need to be true?)
if (!containerNode.inheritPermissions) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends how this system expects it to behave. If it's not, then the system needs to explicitly set the permissions for each node when it gets created. Otherwise, you only set it once in the root directory and the permissions are inherited.

@@ -237,7 +237,7 @@ private List<Node> getOwnedDOIList() throws Exception {
ContainerNode doiRootNode = vospaceDoiClient.getContainerNode("");
if (doiRootNode != null) {
for (Node childNode : doiRootNode.getNodes()) {
// TODO: configure doiadmin viewing of all nodes
// TODO: configure doi admin viewing of all nodes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's available regardless of the permissions down below.

# (optional) Group URI for a group that has read/write permissions to test DOI's
ca.nrc.cadc.doi.test.groupUri = {group URI}
# (optional) Create a random DOI ID for testing
ca.nrc.cadc.doi.randomTestID = {true|false}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary? Can just the prefix and different test directory be enough? Under what circumstances would int testing interfere with production and what would be the consequences?
If you can get rid of this then you can remove traces of test code in the service code as well.

String nextDoiSuffix;
if (randomName) {
if (randomTestID) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be remove if random test id not necessary.

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

Successfully merging this pull request may close these issues.

2 participants