-
-
Notifications
You must be signed in to change notification settings - Fork 615
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
feat(remote): prefix checksums/cached files with the filename #1636
Conversation
Thanks for working on this 🚀 Just a thought on the requirements for this. This was originally raised as a feature to make it easier to distinguish cached files from one another. Suppose we have a setup like the following: version: 3
includes:
a: https://my.remote.com/a/Taskfile.yml
b: https://my.remote.com/b/Taskfile.yml
... This implementation will create two cached files:
These still aren't very distinguishable. Not sure if we want to distinguish them further or if this is "good enough". No strong opinions here - just food for thought. |
I totally agree with you! I did not think about this configuration. The configuration I had in mind was : one folder with multiple Taskfile in it, but we could support multiple folders with one taskfile in it. First solution I had in mind was to put all the path with So, my proposition here is to include only the last directory in the filename, following this pattern version: 3
includes:
a: https://my.remote.com/a/Taskfile.yml
b: https://my.remote.com/b/Taskfile.yml
long: https://raw.githubusercontent.com/my-org/my-repo/main/Taskfile.yml
long_with_dir: https://raw.githubusercontent.com/my-org/my-repo/main/directory/Taskfile.yml
no_dir: https://my.remote.com/Taskfile.yml
... Would be resolved to :
This way we could support almost all the cases. What is your opinion about it ? Add the last dir in the filename or keep it simple with only the filename ? |
@vmaerten Agreed, this was my first thought too, but I think having very long file names defeats the point of making this more readable. It can also cause problems in Windows where there are path length restrictions on some versions.
I think this is probably good enough for now. This is still an experiment, so we can always revisit it later if we have a better idea without worrying about breaking things. |
func (c *Cache) cacheFilePath(node Node) string { | ||
return filepath.Join(c.dir, fmt.Sprintf("%s.yaml", c.key(node))) | ||
return c.filePath(node, "yaml") | ||
} | ||
|
||
func (c *Cache) checksumFilePath(node Node) string { | ||
return filepath.Join(c.dir, fmt.Sprintf("%s.checksum", c.key(node))) | ||
return c.filePath(node, "checksum") | ||
} |
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.
extract a method here because it was almost the same code
6823b53
to
488edf4
Compare
We are on the same page so :) |
I've introduce a new concept in node, the
filename
. This will allow us to store remote checksums/cached files with a more user-friendly filenameLinked to :