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

feat(remote): prefix checksums/cached files with the filename #1636

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

Conversation

vmaerten
Copy link
Collaborator

@vmaerten vmaerten commented May 8, 2024

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 filename

Linked to :

@pd93 pd93 mentioned this pull request May 8, 2024
15 tasks
@pd93
Copy link
Member

pd93 commented May 8, 2024

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:

  • Taskfile.yml.<hash(a)>.yaml
  • Taskfile.yml.<hash(b)>.yaml

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.

@vmaerten
Copy link
Collaborator Author

vmaerten commented May 9, 2024

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 - as separator in the file, in your example it would be a-Taskfile.yml.<hash(a)>.yaml
But the "big" path, I would not fit well, for example (like in our docs) some people (including me) will host the remote Taskfile in Github, leading to huge path (/my-org/my-repo/main/Taskfile.yml).
So this solution is not optimal.

So, my proposition here is to include only the last directory in the filename, following this pattern <lastDir>-<filename>.<hash>.yaml

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 :

  • a-Taskfile.yml.<hash>.yaml
  • b-Taskfile.yml.<hash>.yaml
  • main-Taskfile.yml.<hash>.yaml
  • directory-Taskfile.yml.<hash>.yaml
  • Taskfile.yml.<hash>.yaml

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 ?
I don't know if people include multiple taskfiles from different origins.

@pd93
Copy link
Member

pd93 commented May 10, 2024

First solution I had in mind was to put all the path with - as separator in the file, in your example it would be a-Taskfile.yml.<hash(a)>.yaml But the "big" path, I would not fit well

@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.

So, my proposition here is to include only the last directory in the filename, following this pattern <lastDir>-<filename>.<hash>.yaml

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.

Comment on lines 52 to +58
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")
}
Copy link
Collaborator Author

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

@vmaerten vmaerten force-pushed the refactor/better-filename-cache branch from 6823b53 to 488edf4 Compare May 10, 2024 20:20
@vmaerten
Copy link
Collaborator Author

We are on the same page so :)
I've updated the code

@andreynering andreynering added the area: remote Changes related to remote taskfiles. label May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: remote Changes related to remote taskfiles.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants