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

Abstract image file access #65

Closed
matheusgomes28 opened this issue Mar 25, 2024 · 6 comments
Closed

Abstract image file access #65

matheusgomes28 opened this issue Mar 25, 2024 · 6 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@matheusgomes28
Copy link
Owner

Make the access to the image files happen through an interface. This would make testing the actual business logic of the image endpoints a little easier, so we don't rely on the file system.

For e2e tests, we can still use the filesystem!

This is similar to the Database interface we have, but for the images.

@matheusgomes28 matheusgomes28 added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Mar 25, 2024
@rafiramadhana
Copy link

rafiramadhana commented Mar 26, 2024

Hi @matheusgomes28 , this issue seems interesting to work

However, I have some questions here:

  • Does this issue depends on Image endpoints #64? Or we can start to work by abstracting existing image file access implementation
  • I've read the codebase, it looks like existing image file access implementation is located in ./admin-app/image.go. Which is only in postImageHandler (as in the main branch), is this correct?

@matheusgomes28
Copy link
Owner Author

Hey @rafiramadhana , thanks for your interest in this task !

Yes, it does depend on that task as the get endpoint for the images need to be merged in for us to abstract it. It should be merged as soon as the suggestions in the PR are fixed 😄

@rafiramadhana
Copy link

Yes, it does depend on that task as the get endpoint for the images need to be merged in for us to abstract it. It should be merged as soon as the suggestions in the PR are fixed 😄

Got it. So, let's wait until #64 get merged.

@matheusgomes28
Copy link
Owner Author

#64 is now merged FYI

@rafiramadhana
Copy link

rafiramadhana commented Mar 31, 2024

@matheusgomes28 Hi, I've just skimmed through the codebase, let's assume we will name our interface as MediaVault (as mentioned here)

MediaVault will have 3 methods (this is very open to discussions and suggestions)

type MediaVault interface {
    Add(string) error
    Get(string) ([]byte, error)
    Remove(string) error
}

// Example
err := mv.Add("/file.txt")

b, err := mv.Get("/file.txt")

err := mv.Delete("/file.txt")

MediaVault will be implemented in given endpoints:

ADMIN

  • GET /images/:id -> not needed. no filesystem op
  • POST /images -> .Add
  • DELETE /images/:id -> .Delete

NON-ADMIN

  • GET /data/images/:id -> .Get

Wdyt?

@matheusgomes28
Copy link
Owner Author

matheusgomes28 commented Oct 20, 2024

This has been implemented as a simply static serve to an image directory.

I also apologise for never replying to your message, I probably forgot to 😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants