feat: Support multiple providers for Remote Taskfiles and support directories #1774
Conversation
This data will be used by tests in the following commit, which will clone this branch to test git remotes.
|
I realize this PR is pretty big and I am making many decisions here. If you'd like to talk about this in real-time perhaps on Discord, let me know! |
|
This was meant to be marked as |
|
|
| // NOTE: Uses the directory of the entrypoint (Taskfile), not the current working directory | ||
| // This means that files are included relative to one another | ||
| entrypointDir := filepath.Dir(r.Dir()) | ||
| return filepathext.SmartJoin(entrypointDir, path), nil |
There was a problem hiding this comment.
I'm not sure if this is correct. I am asking about it in #1757 (comment).
This type implements `error` as a pointer. If the type passed in is not a pointer to a pointer, `errors.As()` panics.
Without this, FileNode things that `github.com/org/repo` is a file location and it attempts to map it relative to itself. Something tells me we should integrate both node types into some kind of shared type, but for now this works.
|
I don't love the way this PR turned out. I see two areas that are awkward right now: How the FileNode and RemoteNode integrate with each otherEach node needs to be able to handle relative include paths in its own includes via One idea is to have a single node type that handles file and remote. Another is to refactor this more and separate the path resolution aspect from the download aspect - although I'm not sure how much they can be separate since they depend on each other. The sequence between node loading and cachingCurrently I am storing remote node data in a temp directory until the user accepts it, then we move it to the cache. This means the node's "source" data points to the temp directory and we must reload it if we end up using the cached version (here). I'm trying to think if there's simpler logic there. Caching behaviour in generalWith the ability to download directories, this opens the door to larger downloads. The behaviour of automatically downloading every time feels a bit heavy. I wonder if we could have an option to change the default so that once a remote include is cached, we will always use the cache. For people using versioned refs (eg. In this case two options I see are: |
|
Thanks for your work on this! I would definitely be keen for an option to use the cache by default. To me it ties in nicely to this other feature request I created #1402 |
This PR aims to support multiple providers for Remote Taskfiles using
go-getter, as I proposed in this comment in [#1317]. It is currently marked asDraftas I aim to get some early feedback, and iterate on it if we want to go forward with this approach or something similar.This would support the following shapes of URLs as provided by go-getter (non-exhaustive):
https://example.com/Taskfile.yamlhttps://example.com/some-archive.zipgithub.com/go-task/task//testdata/includes/Taskfile.yml?ref=some_branch[email protected]:repo/pathgitexecutiy using SSH)my-bucket.s3.amazonaws.com/path/to/Taskfile.ymlmy-bucket.s3.amazonaws.com/path/to/folder/my-bucket.s3.amazonaws.com/path/to/archive.zipmy-bucket.s3.amazonaws.com/path/to/archive.zip?taskfile=AnotherTaskfile.ymlThe aims of the change are as follows:
go-getterThese are not directly tied to
go-getterbut they are satisfied by the library fairly well.Some points of note with the current implementation:
There doesn't seem to be a need for multiple remote nodes - as long as one satisfies the
GetterandDetectorinterfaces, one can plug a new provider intoRemoteNode.The contract between
Nodeand the rest of the code is that theNodeplaces its content in a directory and providesFileDirectoryandFilename, which are used for accessing the Taskfile and Taskfile directory and to set variables like.TASKFILE_DIR.FileNodecould eventually be replaced by logic inRemoteNodeas well, if we'd like.go-gettersupports local files by using symlinks and we could leverage this. I'm just not sure how well this would on Windows - I would be glad to test it.The interaction between
Node,ReaderandCachefeels a bit confusing right now in terms of their lifecycle. It feels like we read the node contents a bit too late in the game, and that we could read the contents and deal with caching before we return a Node to the Reader. I didn't try to change this yet - I wanted to get some eyes on the overall idea, then make a plan to refactor it - either in this PR or a future one.ie: instead of calling
NewNode()in the reader and then later callnode.Read(), we would call something liker.loadNode()which already returns a fully loaded Node either from the cache or from remote. In other words, node initialization would include "reading". This would allow us to return a node with an immutableFileDirectory. Currently we have to replace the node if we end up loading its content from the cache because the directory changes.This is also a big PR and I'd be happy to break it up into more manageable pieces if there's consensus. For example, some test and node/cache changes could be done first before adding go-getter.
What do you all think about the above?