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

368: Use Local File Path annotation #370

Open
wants to merge 12 commits into
base: feature/local_cloud_toggle
Choose a base branch
from

Conversation

pgarrison
Copy link
Contributor

@pgarrison pgarrison commented Dec 18, 2024

Purpose

Render the new "Local File Path" annotation nicely. The goal is to display something like this:

File Path (Vast) /mypath/aics/software/apps/staging/fss/data/7fb/6fb/b0f/6bd/7d5/1db/a4c/1e4/2a6/2a5/c7/aicsfiles-test-a4cb9af6-bcfa-11ef-8fe8-0242ac110002.czi

Closes #368
Closes #369

Changes

  • I had to create some logic for renaming the annotation from "Local File Path." I ended up putting it in the HttpAnnotationService, which has a few benefits.
    • The implementation is simpler than putting the logic downstream (e.g., in the FileAnnotationList).
    • The renaming only applies to the FMS data source, so people running BFF from a CSV won't be surprised by it.

Testing

Notes

  • The name File Path (Local) was used before File Path (Vast). I don't have a strong preference: please weigh in. The new FMSAnnotationPresentationLayer makes it easy to change it up.
  • I'm not totally sure of the name FMSAnnotationPresentationLayer nor the choice to put this annotation-renaming logic here instead of FES or changing the name in LabKey and the ETL pipeline.

@lynwilhelm
Copy link
Collaborator

@pgarrison If we are specifically calling out VAST in the "Copy files to local NAS (VAST)" modal @BrianWhitneyAI was working on, perhaps we should be consistent and mention VAST here? Without potentially understanding the whole picture, I usually think you should be more general in terminology so that if VAST changes to something else someday, we won't have to worry about updates. BUT, I read Kevin M's recent comment somewhere that he was happy to see us calling VAST out specifically, which makes me think users have been confused in the past about what our local storage was called? To be consistent, maybe the label should say (Local VAST) or (Local/VAST)?

@pgarrison
Copy link
Contributor Author

pgarrison commented Dec 20, 2024

@pgarrison If we are specifically calling out VAST in the "Copy files to local NAS (VAST)" modal @BrianWhitneyAI was working on, perhaps we should be consistent and mention VAST here? Without potentially understanding the whole picture, I usually think you should be more general in terminology so that if VAST changes to something else someday, we won't have to worry about updates. BUT, I read Kevin M's recent comment somewhere that he was happy to see us calling VAST out specifically, which makes me think users have been confused in the past about what our local storage was called? To be consistent, maybe the label should say (Local VAST) or (Local/VAST)?

@lynwilhelm I realized I uploaded the wrong screenshot -- it's fixed now. The current version of this PR says "File Path (Vast)". Does that seem good to you, or do you think "File Path (Local Vast)" is more clear?

@pgarrison Classic;) IDK, I slightly lean toward calling it local storage too. Whatever you think...but last thing is VAST is an acronym so I think you should do uppercase, right?

@pgarrison
Copy link
Contributor Author

pgarrison commented Dec 21, 2024

Lyndsay left the following comment.

@pgarrison Classic;) IDK, I slightly lean toward calling it local storage too. Whatever you think...but last thing is VAST is an acronym so I think you should do uppercase, right?

So I think this PR needs to be updated to "File Path (Local VAST)" before merging.

@BrianWhitneyAI

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.

5 participants