-
Notifications
You must be signed in to change notification settings - Fork 2
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
bugfix/empty-local-path #458
Conversation
@@ -6,7 +6,7 @@ export default { | |||
FILE_NAME: "file_name", // a file attribute (top-level prop on file documents in MongoDb) | |||
FILE_SIZE: "file_size", // a file attribute (top-level prop on file documents in MongoDb) | |||
FILE_PATH: "file_path", // a file attribute (top-level prop on file documents in MongoDb) | |||
LOCAL_FILE_PATH: "File Path (Local VAST)", // (optional) annotation for FMS files on the local NAS | |||
LOCAL_FILE_PATH: "file_local_path", // (optional) annotation for FMS files on the local NAS |
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 don't understand the need for this? When does "File Path (Local VAST)" come into play?
@@ -110,6 +101,7 @@ export default class FileDetail { | |||
this.fileDetail = fileDetail; | |||
this.env = env; | |||
this.uniqueId = uniqueId; | |||
this.fileDetail.file_local_path = this.localPath; |
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.
Why are we changing the inner fileDetail
here?
if (this.path.startsWith("/allen")) { | ||
return this.path; | ||
if (this.downloadInProgress) { | ||
return "Copying to VAST in progress…"; |
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 like this
@@ -211,7 +188,7 @@ export default class FileDetail { | |||
|
|||
public get downloadInProgress(): boolean { | |||
const shouldBeInLocal = this.getFirstAnnotationValue(AnnotationName.SHOULD_BE_IN_LOCAL); | |||
return Boolean(shouldBeInLocal) && !this.getLocalPath(); | |||
return Boolean(shouldBeInLocal) && !this.getAnnotation("Cache Eviction Date"); |
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.
Looks like now that it is used a couple places "Cache Eviction Date" should be added to AnnotationName
and used like a constant
} | ||
//// | ||
|
||
if (this.getAnnotation("Cache Eviction Date")) { | ||
return this.getLocalPath(); |
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.
Can we just move the body of the getLocalPath
function in here? Feels funky that there are two localPath functions.
Like so:
const cloudPrefix = `${AICS_FMS_S3_URL_PREFIX}${AICS_FMS_S3_BUCKETS[this.env]}`;
if (this.getAnnotation("Cache Eviction Date") && this.path.startsWith(cloudPrefix)) {
const localPrefix = NAS_HOST_PREFIXES[this.env];
const relativePath = this.path.replace(`${cloudPrefix}`, "");
return `${localPrefix}${relativePath}`;
}
return null;
Context
Changes
Testing