-
-
Notifications
You must be signed in to change notification settings - Fork 624
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: enable root remote taskfiles #1347
Conversation
Hi @pd93, I would love to use this feature with the other changes that are in |
8d03ad3
to
f2d66a6
Compare
@pd93 just following up again on this PR. Is there feedback (workflow usage details) or coding support I could provide to help get this work across the line? |
f2d66a6
to
796f435
Compare
796f435
to
aa9e86e
Compare
aa9e86e
to
2cf4a41
Compare
2cf4a41
to
75d94ef
Compare
75d94ef
to
6e6e262
Compare
This PR has taken a long time to complete 😆 I did not initially realise the complexity of the changes that would be required to complete this work. Over the last couple of months or so I have spent many hours reworking this from the ground up. The functionality described in the PR description required several major changes to some core code and I wanted to make sure I'm not breaking anything for existing users. I've spent the last couple of days testing. I suspect there will still be some issues, but the main changes are now complete and ready to be reviewed 🚀 @vmaerten @wburningham Since you've both been trying out remote Taskfiles recently, would you mind kicking the tyres on this for me? I'm pretty sure it's ready to go now. This should resolve both remote root Taskfiles and includes in remote Taskfiles. |
@brianbraunstein Since this PR also implements the ability to use both |
@pd93 For the includes in remote Taskfiles, I've tested it with two differents setup and it works as expected |
f420aca
to
3de5e97
Compare
I got around to testing this. I ran
version: 3 tasks:
|
Hey @wburningham. Special variables for remote Taskfiles are something I haven't worked on yet. It seems they're pretty broken right now. I will add this to the todo list in #1317 and take a look at these in a future PR. As long as the ability to access root files remotely and to include other remote files in a remote file is working correctly then I'm happy to merge this as-is.
To answer your question directly. |
I think I found a bug related to the special variables. I can see Steps to reproduce:
Notice how |
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.
🚀
locally at all. For example: | ||
|
||
```shell | ||
task --taskfile https://taskfile.dev hello |
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.
We could consider having a proper Taskfile being served in the website, so copying and pasting would work 🙂
Could be a GitHub or GitHub Gist URL as well...
task --taskfile https://taskfile.dev hello | |
task --taskfile https://taskfile.dev/taskfiles/hello.yml hello |
Just an idea, does not need to be done on this PR...
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.
Not entirely clear what you mean by this
We could consider having a proper Taskfile being served in the website
Is this not already complete in this PR? see https://github.com/go-task/task/pull/1347/files#diff-eea9e58f9afa47c7cdf1393b2d383a4713f91171f6603ced3937cacc09fe9376
I will merge for now and we can adjust later if need be.
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.
Oh, got it, it's already there in the root 🙂
It just surprised me that it automatically fetches the /Taskfile.yml
path. I thought the whole path would have to be explicit...
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.
This is what the RemoteExists
function is doing. It's similar to what we do with local files where only a directory is given. Task will look around for a file that makes sense. I thought this would be nice for projects that want to use Taskfiles to do some setup work. e.g.
task --taskfile https://myproject.org init
or something similar is a really nice, clean way of asking your devs to do run some setup task. Whereas task --taskfile https://myproject.org/Taskfile.yml init
is a bit less tidy.
The same applies to the new taskfile.dev/Taskfile.yml
file. If a user wants to check that their installation of Task works, they can now run task --taskfile https://taskfile.dev
and it will reply with "Hello Task!". This is a bit less clunky than task --taskfile https://taskfile.dev/Taskfile.yml
.
Happy to revisit this if you don't like the behaviour though.
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's actually an interesting feature, we can keep it 👍
I did quick review at the time, I so end up not paying attention to that function 🙂
I just suggest to improve the documentation a bit to explain that this is a possibility, just like for local files.
taskfile/taskfile.go
Outdated
} | ||
|
||
// Request the given URL | ||
resp, err := http.DefaultClient.Do(req) |
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.
Does these HTTP calls have a timeout by default? It would be desired IMO.
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.
Good shout! I've added the existing timeout functionality in the HTTO node.Read(...)
to taskfile.RemoteExists(...)
8ef8d45
to
b346ace
Compare
Fixes #1531
This PR enables the ability to pass a remote Taskfile URI to the
--taskfile
flag. This means that your root Taskfile no longer needs to exist locally.When specifying the
--taskfile
flag, Task usually assumes that the execution directory is the directory that the root Taskfile resides in. Since this isn't possible with a remote Taskfile, the execution directory will default to the current working directory instead when the Task command is executed.However, I have also added the ability to change this default directory using the existing
--dir
flag. This now means that you are able to specify BOTH--taskfile
and--dir
flags simultaneously - Something that was not previously allowed. The following semantics are now applied:--taskfile
is specified without--dir
, the specified Taskfile is used and the directory will default to the directory containing the specified Taskfile (no change).--dir
is specified without--taskfile
, Task will search the specified directory for a Taskfile and use that as the entrypoint. The directory will be the one specified (no change).--dir
and--taskfile
are specified, the entrypoint will be the specified taskfile and the directory will be the specified directory.Finally, this PR also fixes issues related to resolving the location of included Taskfiles inside remote Taskfiles. For example, if you are using a remote Taskfile that includes a "local" file. The included file will now be resolved as a remote file too and the include will work as expected.