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

bugfix/empty-local-path #458

Closed
wants to merge 2 commits into from
Closed

Conversation

BrianWhitneyAI
Copy link
Contributor

Context

Changes

Testing

@@ -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
Copy link
Contributor

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;
Copy link
Contributor

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…";
Copy link
Contributor

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");
Copy link
Contributor

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();
Copy link
Contributor

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;

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