You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@airavata.apache.org by GitBox <gi...@apache.org> on 2021/02/02 21:22:39 UTC
[GitHub] [airavata-mft] machristie opened a new pull request #28: Fixes for refactored storages api
machristie opened a new pull request #28:
URL: https://github.com/apache/airavata-mft/pull/28
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airavata-mft] machristie commented on a change in pull request #28: Fixes for refactored storages api
Posted by GitBox <gi...@apache.org>.
machristie commented on a change in pull request #28:
URL: https://github.com/apache/airavata-mft/pull/28#discussion_r573307780
##########
File path: transport/scp-transport/src/main/java/org/apache/airavata/mft/transport/scp/SCPMetadataCollector.java
##########
@@ -203,17 +204,17 @@ public DirectoryResourceMetadata getDirectoryResourceMetadata(String resourceId,
}
@Override
- public DirectoryResourceMetadata getDirectoryResourceMetadata(String storageId, String resourcePath, String credentialToken) throws Exception {
+ public DirectoryResourceMetadata getDirectoryResourceMetadata(String resourceId, String resourcePath, String credentialToken) throws Exception {
ResourceServiceClient resourceClient = ResourceServiceClientBuilder.buildClient(resourceServiceHost, resourceServicePort);
- SCPStorage scpStorage = resourceClient.scp().getSCPStorage(SCPStorageGetRequest.newBuilder().setStorageId(storageId).build());
+ SCPResource scpPResource = resourceClient.scp().getSCPResource(SCPResourceGetRequest.newBuilder().setResourceId(resourceId).build());
SecretServiceClient secretClient = SecretServiceClientBuilder.buildClient(secretServiceHost, secretServicePort);
SCPSecret scpSecret = secretClient.scp().getSCPSecret(SCPSecretGetRequest.newBuilder().setSecretId(credentialToken).build());
-
- SCPResource scpResource = SCPResource.newBuilder().setScpStorage(scpStorage)
- .setDirectory(DirectoryResource.newBuilder()
- .setResourcePath(resourcePath).build()).build();
+ String childPath = Paths.get(scpPResource.getDirectory().getResourcePath(), resourcePath).toString();
Review comment:
@DImuthuUpe I've fixed the resourcePath to just be the received childPath, as we discussed last week.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airavata-mft] machristie commented on pull request #28: Fixes for refactored storages api
Posted by GitBox <gi...@apache.org>.
machristie commented on pull request #28:
URL: https://github.com/apache/airavata-mft/pull/28#issuecomment-777775938
@DImuthuUpe I've added another fix, but I'm not sure if this is right. I know that SCPMetadataCollector.getFileResourceMetadata(String storageId, String resourcePath, String credentialToken) is getting called with the resourceId, not the storageId as the first argument, so I've fixed the implementation to work with that. But I noticed that you recently updated the interface of getFileResourceMetadata to take storageId (was: parentResourceId) as the first argument. So perhaps the bug is really in the code calling getFileResourceMetadata?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airavata-mft] machristie commented on a change in pull request #28: Fixes for refactored storages api
Posted by GitBox <gi...@apache.org>.
machristie commented on a change in pull request #28:
URL: https://github.com/apache/airavata-mft/pull/28#discussion_r568941520
##########
File path: transport/scp-transport/src/main/java/org/apache/airavata/mft/transport/scp/SCPMetadataCollector.java
##########
@@ -203,17 +204,17 @@ public DirectoryResourceMetadata getDirectoryResourceMetadata(String resourceId,
}
@Override
- public DirectoryResourceMetadata getDirectoryResourceMetadata(String storageId, String resourcePath, String credentialToken) throws Exception {
+ public DirectoryResourceMetadata getDirectoryResourceMetadata(String resourceId, String resourcePath, String credentialToken) throws Exception {
ResourceServiceClient resourceClient = ResourceServiceClientBuilder.buildClient(resourceServiceHost, resourceServicePort);
- SCPStorage scpStorage = resourceClient.scp().getSCPStorage(SCPStorageGetRequest.newBuilder().setStorageId(storageId).build());
+ SCPResource scpPResource = resourceClient.scp().getSCPResource(SCPResourceGetRequest.newBuilder().setResourceId(resourceId).build());
SecretServiceClient secretClient = SecretServiceClientBuilder.buildClient(secretServiceHost, secretServicePort);
SCPSecret scpSecret = secretClient.scp().getSCPSecret(SCPSecretGetRequest.newBuilder().setSecretId(credentialToken).build());
-
- SCPResource scpResource = SCPResource.newBuilder().setScpStorage(scpStorage)
- .setDirectory(DirectoryResource.newBuilder()
- .setResourcePath(resourcePath).build()).build();
+ String childPath = Paths.get(scpPResource.getDirectory().getResourcePath(), resourcePath).toString();
Review comment:
@DImuthuUpe I wasn't sure how childPath should be used. I assumed that it would be concatenated with the scp resource's resourcePath. For example, if the scp resource has resourcePath `/tmp` and the childPath for the FetchResourceMetadataRequest is `test`, then I would expect that these would be concatenated and the result would be metadata for the directory `/tmp/test`, so that's what I implemented. But let me know if I got the idea wrong.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airavata-mft] DImuthuUpe commented on pull request #28: Fixes for refactored storages api
Posted by GitBox <gi...@apache.org>.
DImuthuUpe commented on pull request #28:
URL: https://github.com/apache/airavata-mft/pull/28#issuecomment-777925941
@machristie This is an intentional change. There are 2 overloaded methods for getDirectoryResourceMetadata
public DirectoryResourceMetadata getDirectoryResourceMetadata(String resourceId, String credentialToken)
public DirectoryResourceMetadata getDirectoryResourceMetadata(String storageId, String resourcePath, String credentialToken)
The first one is when you have a resource registered in the resource service. The second one is when you don't have a resource registered but you need to list files. I came up with the second one to avoid the overhead of registering resources for each storage but I see some possible issues with authorization. Maybe we need to change it back to
public DirectoryResourceMetadata getDirectoryResourceMetadata(String parentResourceId, String resourcePath, String credentialToken)
What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airavata-mft] DImuthuUpe merged pull request #28: Fixes for refactored storages api
Posted by GitBox <gi...@apache.org>.
DImuthuUpe merged pull request #28:
URL: https://github.com/apache/airavata-mft/pull/28
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airavata-mft] machristie commented on pull request #28: Fixes for refactored storages api
Posted by GitBox <gi...@apache.org>.
machristie commented on pull request #28:
URL: https://github.com/apache/airavata-mft/pull/28#issuecomment-780872775
@DImuthuUpe I've switched the methods back to taking a parentResourceId in the latest commit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airavata-mft] machristie commented on a change in pull request #28: Fixes for refactored storages api
Posted by GitBox <gi...@apache.org>.
machristie commented on a change in pull request #28:
URL: https://github.com/apache/airavata-mft/pull/28#discussion_r568941520
##########
File path: transport/scp-transport/src/main/java/org/apache/airavata/mft/transport/scp/SCPMetadataCollector.java
##########
@@ -203,17 +204,17 @@ public DirectoryResourceMetadata getDirectoryResourceMetadata(String resourceId,
}
@Override
- public DirectoryResourceMetadata getDirectoryResourceMetadata(String storageId, String resourcePath, String credentialToken) throws Exception {
+ public DirectoryResourceMetadata getDirectoryResourceMetadata(String resourceId, String resourcePath, String credentialToken) throws Exception {
ResourceServiceClient resourceClient = ResourceServiceClientBuilder.buildClient(resourceServiceHost, resourceServicePort);
- SCPStorage scpStorage = resourceClient.scp().getSCPStorage(SCPStorageGetRequest.newBuilder().setStorageId(storageId).build());
+ SCPResource scpPResource = resourceClient.scp().getSCPResource(SCPResourceGetRequest.newBuilder().setResourceId(resourceId).build());
SecretServiceClient secretClient = SecretServiceClientBuilder.buildClient(secretServiceHost, secretServicePort);
SCPSecret scpSecret = secretClient.scp().getSCPSecret(SCPSecretGetRequest.newBuilder().setSecretId(credentialToken).build());
-
- SCPResource scpResource = SCPResource.newBuilder().setScpStorage(scpStorage)
- .setDirectory(DirectoryResource.newBuilder()
- .setResourcePath(resourcePath).build()).build();
+ String childPath = Paths.get(scpPResource.getDirectory().getResourcePath(), resourcePath).toString();
Review comment:
@DImuthuUpe I wasn't sure how childPath should be used. I assumed that it would be concatenated with the scp resource's resourcePath. For example, if the scp resource has resourcePath `/tmp` and the childPath for the FetchResourceMetadataRequest is `test`, then I would expect that these would be concatenated and the result would be metadata for the directory `/tmp/test`, so that's what I implemented. But let me know if I got the idea wrong.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airavata-mft] machristie commented on a change in pull request #28: Fixes for refactored storages api
Posted by GitBox <gi...@apache.org>.
machristie commented on a change in pull request #28:
URL: https://github.com/apache/airavata-mft/pull/28#discussion_r570487239
##########
File path: transport/scp-transport/src/main/java/org/apache/airavata/mft/transport/scp/SCPMetadataCollector.java
##########
@@ -203,17 +204,17 @@ public DirectoryResourceMetadata getDirectoryResourceMetadata(String resourceId,
}
@Override
- public DirectoryResourceMetadata getDirectoryResourceMetadata(String storageId, String resourcePath, String credentialToken) throws Exception {
+ public DirectoryResourceMetadata getDirectoryResourceMetadata(String resourceId, String resourcePath, String credentialToken) throws Exception {
ResourceServiceClient resourceClient = ResourceServiceClientBuilder.buildClient(resourceServiceHost, resourceServicePort);
- SCPStorage scpStorage = resourceClient.scp().getSCPStorage(SCPStorageGetRequest.newBuilder().setStorageId(storageId).build());
+ SCPResource scpPResource = resourceClient.scp().getSCPResource(SCPResourceGetRequest.newBuilder().setResourceId(resourceId).build());
SecretServiceClient secretClient = SecretServiceClientBuilder.buildClient(secretServiceHost, secretServicePort);
SCPSecret scpSecret = secretClient.scp().getSCPSecret(SCPSecretGetRequest.newBuilder().setSecretId(credentialToken).build());
-
- SCPResource scpResource = SCPResource.newBuilder().setScpStorage(scpStorage)
- .setDirectory(DirectoryResource.newBuilder()
- .setResourcePath(resourcePath).build()).build();
+ String childPath = Paths.get(scpPResource.getDirectory().getResourcePath(), resourcePath).toString();
Review comment:
I discussed with @DImuthuUpe and decided that the childPath should be the full path. Actually, the childPath value passed should be the same as the resourcePath returned for that resource when the listing was returned for the parent. That is, the API client can call getDirectoryResourceMetadata and take the resourcePath for each returned directory and then call getDirectoryResourceMetadata with that resourcePath as the childPath value.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airavata-mft] machristie commented on a change in pull request #28: Fixes for refactored storages api
Posted by GitBox <gi...@apache.org>.
machristie commented on a change in pull request #28:
URL: https://github.com/apache/airavata-mft/pull/28#discussion_r570487239
##########
File path: transport/scp-transport/src/main/java/org/apache/airavata/mft/transport/scp/SCPMetadataCollector.java
##########
@@ -203,17 +204,17 @@ public DirectoryResourceMetadata getDirectoryResourceMetadata(String resourceId,
}
@Override
- public DirectoryResourceMetadata getDirectoryResourceMetadata(String storageId, String resourcePath, String credentialToken) throws Exception {
+ public DirectoryResourceMetadata getDirectoryResourceMetadata(String resourceId, String resourcePath, String credentialToken) throws Exception {
ResourceServiceClient resourceClient = ResourceServiceClientBuilder.buildClient(resourceServiceHost, resourceServicePort);
- SCPStorage scpStorage = resourceClient.scp().getSCPStorage(SCPStorageGetRequest.newBuilder().setStorageId(storageId).build());
+ SCPResource scpPResource = resourceClient.scp().getSCPResource(SCPResourceGetRequest.newBuilder().setResourceId(resourceId).build());
SecretServiceClient secretClient = SecretServiceClientBuilder.buildClient(secretServiceHost, secretServicePort);
SCPSecret scpSecret = secretClient.scp().getSCPSecret(SCPSecretGetRequest.newBuilder().setSecretId(credentialToken).build());
-
- SCPResource scpResource = SCPResource.newBuilder().setScpStorage(scpStorage)
- .setDirectory(DirectoryResource.newBuilder()
- .setResourcePath(resourcePath).build()).build();
+ String childPath = Paths.get(scpPResource.getDirectory().getResourcePath(), resourcePath).toString();
Review comment:
I discussed with @DImuthuUpe and decided that the childPath should be the full path. Actually, the childPath value passed should be the same as the resourcePath returned for that resource when the listing was returned for the parent. That is, the API client can call getDirectoryResourceMetadata and take the resourcePath for each returned directory and then call getDirectoryResourceMetadata with that resourcePath as the childPath value.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org