Extend ImportArtifact to to import Arbitrary Paths in Storage Backends (Closes #1608) #1630
Conversation
…tension 1. Add stat method in sciserver-files 2. Complete plugin for transfering arbitrary paths 3. Write tests for stat method in StorageFeatures.spec.js
| icon: 'swap_vert', | ||
| action: DeepForge.create.Artifact | ||
| action: function() { | ||
| DeepForge.create.Artifact('dataPath'); |
There was a problem hiding this comment.
I am not a fan of this method signature as I find it a bit unnatural. It's odd that the string passed corresponds to a key in the config to omit. My initial reaction was that the string passed was specifying the way in which the artifact was being referenced.
I think it would probably be more natural to just filter the configStructure (or entire metadata) here and then pass that as the input to the argument.
…fo in s3 storage's stat method
brollb
left a comment
brollb
left a comment
There was a problem hiding this comment.
It might be nice to add some unit tests, too, though it isn't clear how easy that would be given the lack of a storage adapter that supports stat without requiring us spinning up something like minio. That said, it isn't blocking this PR.
src/common/plugin/Artifacts.js
Outdated
| ], function( | ||
| ) { | ||
| const Artifacts = function () { | ||
| throw new Error('Aritifacts is supposed to be used as an extension object, do not instantiate'); |
There was a problem hiding this comment.
typo: Aritifacts -> Artifacts
src/common/plugin/Artifacts.js
Outdated
| return base; | ||
| }; | ||
|
|
||
| Artifacts.prototype.createArtifact = async function (baseNode, attrs) { |
There was a problem hiding this comment.
Why does this accept baseNode as an input? Isn't this always Data from the metamodel?
| } | ||
|
|
||
| S3Storage.prototype.stat = async function (path) { | ||
| const metadata = await this._stat(path, this.bucketName); |
There was a problem hiding this comment.
It might be nice to remove _stat as it is only used in stat to simplify the code a little bit (the method name isn't terribly informative). Not a huge deal though
src/common/plugin/Artifacts.js
Outdated
|
|
||
| Artifacts.prototype.constructor = Artifacts; | ||
|
|
||
| Artifacts.prototype.getBaseNode = function (baseName) { |
There was a problem hiding this comment.
I don't think adding a parameter (which is always "Data") is the right solution here. This could be renamed to something like getArtifactMetaNode though I think a better solution would be to change the method to perform the entire check that is duplicated in both plugins. Something like ensureCompatibleMeta which is an async method that throws the an error with the message returned in the callback. Then each plugin could run that first in their main function.
There was a problem hiding this comment.
This could mean that we are just adding a new method btw. This method doesn't need to be removed as it could still be used by the other methods in this file. That said, it should still probably be renamed :)
src/common/globals.js
Outdated
| const runArtifactPlugin = async function(pluginName) { | ||
| const pluginMetadata = copy(WebGMEGlobal.allPluginsMetadata[pluginName]); | ||
| const backends = pluginName === UPLOAD_PLUGIN ? storageBackends : | ||
| storageBackends.filter(backend => backend !== 's3'); |
There was a problem hiding this comment.
Shouldn't this be removing the "gme" backend?
1. Fix stat method in s3Storage to use a single function 2. Change 's3' from filter to 'gme' in globals.js 3. Add 'ensureCompatibleMeta' in Artifacts.js 4. Fix typo in ImportArtifact 5. Refactor getBaseNode to getArtifactMetaNode in Artifacts.js
|
After playing with it a bit, I think there is a better way to display the config info. It seemed a little unclear what the path was referring to when it was part of its own section and seemed more fitting to simply be an additional field for every storage backend. I updated the config dialog accordingly:
|
|
The order for the uploading plugin still makes sense as there is no connection between the uploaded artifact and the storage backend. |
2 participants
To Dos:
S3 StorageBackend.SciServer-Files.