feat: Add caching for remote socket fetch with configurable TTL

[fmich7]

Hi @Tackx 🙋🏻‍♂️

This PR adds caching for remote socket fetches, addressing this task (dish should cache remote socket list to some file #3).

What I did

• Added the following flags:
  • cache Enables caching
  • cacheDir=[string] Specify the directory where cached files are stored
  • cacheTTL=[uint] Set the cache expiration time in minutes
• Caching behavior:
  • We check if we already have a cache for the specified URL:
    • If we do and the TTL is not expired, use sockets from the cache
    • If we do and the TTL is expired, attempt to fetch fresh ones from the network and refresh the cache with them
      • If the fetch from the network fails, use the cached sockets even though their TTL is expired (logs a warning)
  • If we do not, attempt to fetch from the network. If it fails, there is nothing we can do, return an error
• Cleaned up socket package codebase
• Added testhelpers package
• Wrote tests for sockets package (also with caching)

To-do

• Write tests
• Update README.md

Also, you might consider changing the way configuration variables are passed in this project. It's inconvenient to pass that many variables to functions.

Please review and provide feedback for this PR so I can make any necessary improvements.

[Tackx]

Hi @fmich7,

Thank you for this!

Overall it is looking good. I appreciate the refactor as well.

I have added comments prefixed with TODO: to be processed.

I found one legitimate issue where the sockets are loaded from cache even when the -cache flag is not used. However, I think we should utilize the cache more than just if the remote API endpoint is down. I have summarized how/when to use cache in one of the comments.

I agree about the configuration variables. I was thinking we could just pass around a pointer to the Config struct and the downstream recipient would then access the fields it needs. What do you think?

[fmich7]

Hey @Tackx,

I went through your feedback and fixed the issues you pointed out. I also updated the documentation and tested the entire package. Coverage is around 89.2%, which is solid.

Additionally, I added a testhelpers package to keep test utils in one place. It includes a function for easy temp file creation and mock endpoints (that we expect users to have on their API). Take a look at this.

I agree about the configuration variables. I was thinking we could just pass around a pointer to the Config struct and the downstream recipient would then access the fields it needs. What do you think?

I am on board with that approach. It will make the codebase a lot easier to understand and expand.

Let me know what you think.

[Tackx]

Hi @fmich7,

It looks very solid now, thank you! I've played around with our remote API endpoint and the cache and it seems to be doing what it should.

The biggest issues I ran into were Windows-specific and only relevant to tests (see the list below). I was able to fix the issues and tests run properly on my machine as well.

I have mostly made just semantic changes and nitpicky corrections with a few small exceptions:

• Added cacheTime to the values returned by loadCachedSockets() so the time of update (realistically the creation time) of the cache can be logged when using expired cache
• Updated some comments, capitalization and general semantics
• Remove unnecessary newlines (the log package inserts a line break at the end of Print() and Printf() just like Println(), the fmt package does not
  • t.Error()/t.Errorf() do not, however, if there are different test cases or subtests between them, a new line will be inserted anyway (I think!)
• Fixed Windows-specific issues I ran into during the tests (mainly the filepaths being different (/ vs \) and errors with cleaning up the temporary files (see testing: t.TempDir() doesn't remove tmp dir and files as expected on Windows OS golang/go#50510))
• Changed the default cache dir to .cache which I think is more consistent with the fact that it is supposed to be a "hidden" directory which the user is not supposed to interact with
• Changed direct error comparison to errors.Is() which I suppose is the more idiomatic (I said it) and modern way (also helpful if we decide to wrap the error in the future).

I think it is good to merge now, so thank you very much again for your help!

If you would like to help with refactoring the config variables to a pointer to the Config struct, I will create an issue for it.

FYI @krustowski

[fmich7]

Hi @Tackx,

Thank you so much for such detailed feedback. I really appreciate it!

The changes you made make sense. Overall, I missed a few things while working on this PR, but your feedback really helped me understand what to watch out for next time, so thanks again!

If you would like to help with refactoring the config variables to a pointer to the Config struct, I will create an issue for it.

Yes, I would be happy to help with refactoring, feel free to open an issue.