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

Add TTL to taskfile.Cache #1702

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

jlucktay
Copy link

@jlucktay jlucktay commented Jun 30, 2024

Description

This PR implements part of the remote Taskfiles experiment (#1317) that is currently ongoing.

Implementation notes

  • taskfile.Cache has a new private time.Duration field for the TTL setting.

    • The default TTL setting for the cache is 24 hours. Should this be shorter/longer? 4 hours? 1 week?
  • The taskfile package has some new exports:

    • the default TTL value described above, in a const.
    • a static error: ErrExpired.
    • a CacheOption type, and a WithTTL func returning this type, to help with creation of new Cache structs.
  • The main task executable has a new flag (currently gated by the TASK_X_REMOTE_TASKFILES experiment) to override the default cache TTL setting, outlined below.

Flow diagram

There is a flowchart sketched out here on this branch to describe the overall flow.

New CLI behaviour

$ TASK_X_REMOTE_TASKFILES=1 task --help 
…
Options:
…
      --remote-cache-ttl duration   TTL of remote Taskfiles downloaded into the local cache. (default 24h0m0s)

Resolves #1402.

@jlucktay jlucktay force-pushed the issue-1402/add-ttl-to-taskfile-cache branch from 69629d0 to 6b2ea85 Compare July 4, 2024 08:39
taskfile/cache.go Outdated Show resolved Hide resolved
taskfile/cache_internal_test.go Outdated Show resolved Hide resolved
taskfile/cache_internal_test.go Outdated Show resolved Hide resolved
taskfile/cache_test.go Outdated Show resolved Hide resolved
taskfile/cache_internal_test.go Outdated Show resolved Hide resolved
@jlucktay
Copy link
Author

jlucktay commented Jul 4, 2024

Thank you for the notes @ccoVeille 🙂 🙏
I'll give things a once-over soon.

@jlucktay jlucktay force-pushed the issue-1402/add-ttl-to-taskfile-cache branch from ea22989 to 6230c71 Compare July 11, 2024 08:05
@jlucktay jlucktay marked this pull request as ready for review July 11, 2024 10:35
@jlucktay jlucktay mentioned this pull request Jul 11, 2024
15 tasks
var (
SLEEPIT, _ = filepath.Abs("./bin/sleepit")
)
var SLEEPIT, _ = filepath.Abs("./bin/sleepit")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you are simply applying a gofumpt on this.

I'm surprised to see:

  • the error is ignored
  • this variable name is in uppercase
  • the variable is apparently used only in one test.

I would expect this variable to be used only where needed and to check the possible error

taskfile/cache.go Outdated Show resolved Hide resolved
@jlucktay jlucktay requested a review from vmaerten September 11, 2024 08:19
Copy link
Member

@vmaerten vmaerten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi !
Thanks for working on this 🔥 To me 24h is too short.
I'll take my example but it can work for other.

We include only taskfiles we own in our company, so I know when I need to fetch a new version. I am always running with offline mode to avoid unecessary http call (and it's faster to not fetch X differents remote taskfiles).

I think we could add an env variable to control the cache duration (this way we do not need to worry about passing the flag each time we download a remote file)

One more thing, the chart does not reflect the real behavior.
image
As we said, the "offline" first will be done in another PR, you need to update the chart

@pd93 any thought on this?

}

func NewCache(dir string) (*Cache, error) {
// ErrExpired is returned when a cached file has expired.
var ErrExpired = errors.New("task: cache expired")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should go in the errors package.
You need to be a bit more specific in the error message, for example write wich file is expired

@vmaerten vmaerten requested a review from pd93 September 15, 2024 13:33
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.

Feature Request: Have an auto update ttl for downloaded taskfiles
3 participants