-
-
Notifications
You must be signed in to change notification settings - Fork 625
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
base: main
Are you sure you want to change the base?
Add TTL to taskfile.Cache #1702
Conversation
69629d0
to
6b2ea85
Compare
Thank you for the notes @ccoVeille 🙂 🙏 |
Mapped out a flowchart to describe the behaviour of taskfile.Cache, so as to aid technical design and implementation. re go-task#1402
ea22989
to
6230c71
Compare
Per the first bullet on this list: https://taskfile.dev/contributing/#2-making-changes
website/versioned_docs/version-latest/experiments/remote_taskfiles.mdx
Outdated
Show resolved
Hide resolved
var ( | ||
SLEEPIT, _ = filepath.Abs("./bin/sleepit") | ||
) | ||
var SLEEPIT, _ = filepath.Abs("./bin/sleepit") |
There was a problem hiding this comment.
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
There was a problem hiding this 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.
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") |
There was a problem hiding this comment.
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
Description
This PR implements part of the remote Taskfiles experiment (#1317) that is currently ongoing.
Implementation notes
taskfile.Cache
has a new privatetime.Duration
field for the TTL setting.The
taskfile
package has some new exports:const
.error
:ErrExpired
.CacheOption
type, and aWithTTL
func returning this type, to help with creation of newCache
structs.The main
task
executable has a new flag (currently gated by theTASK_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
Resolves #1402.