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): support include git remote #1652

Merged
merged 11 commits into from
Sep 24, 2024
Merged

Conversation

vmaerten
Copy link
Member

@vmaerten vmaerten commented May 11, 2024

I've work on add the support of Git in the remote experimentation.
I've used the go-git package, which is Git implementation in pure go.
I've also used git-urls to help me to parse all URLs.

Some informations :

  • I did not handle the custom timeout for now
  • We support SCP / SSH / HTTPS URL
  • It seems that SSH takes more time than HTTPS
  • Git remote entrypoint (--taskfile) works
  • include local -> remote -> remote:local works

I'll work next on perf optimisation. I've in mind to mutualise git clone to avoid cloning multiple times the same repo.

I've added tests. I'll add test of node_http in another PR.

EDIT : The link with to the preview

@vmaerten vmaerten marked this pull request as ready for review May 11, 2024 13:22
@vmaerten
Copy link
Member Author

I've tested it and a friend also tested it. I am open to gather feedback on it :)

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

pd93 commented May 11, 2024

@vmaerten Thanks for working on this! This is a very popular request. I will definitely give this a go in the next couple of days. Finally some nice weather in the UK, so I'll be out enjoying the sun as much as possible this weekend! 🌞

It might be worth putting a message on the remote experiment issue so that people who are subscribed over there can try it too. I have updated the OP there to link back here.

@vmaerten
Copy link
Member Author

I really enjoy working on Task. Let me know if you have questions regarding my implementation
Sure, enjoy the sun, I did the same this weekend in France! 🙂

Good idea! I've just done it 🙂

@vmaerten
Copy link
Member Author

@blackjid 's comment :

That is awesome @vmaerten thank for the work...

Why did you choose to set the filename and the git reference in the same querystring in the url? Why not just having a
different querystring for the filename? https://github.com/foo/bar.git?ref=main&filename=directory/Taskfile.yml

Or if you don't want the extra query string, a more readable form I think could be to add the path to the file just after the .git https://github.com/foo/bar.git/directory/Taskfile.yml?ref=main maybe with a double slash // like terraform and others does.. https://github.com/foo/bar.git//directory/Taskfile.yml?ref=main

Sorry if this is not the place or time to give feedback. Maybe discord?

First of all, thanks for the feedback.
To be honest, I did not find a consensus for this kind of URL in the community, so I took the example URL in the issue (here)

Following your propostions, I like the one with the query param representing the filename. In the other hand, if the community already knows the "terraform" format, it could be good to adopt it.

Let's discuss with @pd93 and @andreynering

@vmaerten vmaerten requested review from pd93 and andreynering May 14, 2024 16:04
@andreynering andreynering added the area: remote Changes related to remote taskfiles. label May 16, 2024
@adlnc
Copy link

adlnc commented Jun 1, 2024

Hi,

I find this work awesome ! Just wanted to mention that I've been able to run this on my side using http over public repo, private repo using token auth and ssh. All behaved as expected.

This, added to the remote file feature will definitely help saving a tremendous amount of time sharing and managing taskfiles. I'm really excited to have this available in a few.

Thanks for this work.

@vmaerten
Copy link
Member Author

vmaerten commented Jun 3, 2024

Thanks for checking this!
We will definitely move forward on this!

@jylenhofdecat
Copy link

Any update ?

Copy link
Member

@pd93 pd93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally pretty happy with this. Just one minor comment.

w.r.t @blackjid's comment:

There have been a lot of comments in the past about reusing the go-getter syntax as it seems pretty popular in the Go community. We're not using go-getter right now, but using a compatible syntax would make it easier to transition to it in the future if we wanted to for any reason.

taskfile/node.go Outdated Show resolved Hide resolved
@vmaerten
Copy link
Member Author

I need to rebase due to recent merged PR
I will do it

@vmaerten
Copy link
Member Author

@pd93 I've rebased to add the function for prefixing the cache (done here: #1636).
I've also taken your review into account.
For some reason, the CI is not triggered. :'(

@vmaerten vmaerten requested a review from pd93 June 28, 2024 17:28
Copy link
Contributor

@blackjid blackjid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @vmaerten !

}

func TestGitNode_sshWithDir(t *testing.T) {
node, err := NewGitNode("git@github.com:foo/bar.git?ref=main//directory/Taskfile.yml", "", false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but using a compatible syntax would make it easier to transition to it in the future if we wanted to for any reason.

I did't thought about it, that would be a compelling reason to use that syntax.

I don't think this new change syntax is compatible...

git@github.com:foo/bar.git?ref=main//directory/Taskfile.yml

should be

git@github.com:foo/bar.git//directory/Taskfile.yml?ref=main

so ref is directly mapped to a git refeference, could be a branch or a tag for example.
id's say you'd need to split the url in // to get the git url at 0 and the filename at 1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @vmaerten thanks for all the work on this!

just want to add that with this syntax, the ref query string should be optional, because, it will only reference to a branch or tag, and without it it would be the default branch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey!
You're right! I'll work on this :)

@oliver-helix
Copy link

oliver-helix commented Jul 30, 2024

What am I doing wrong?

$ export TASK_X_REMOTE_TASKFILES=1
$ task --list --taskfile git@github.com/$ORGANIZATION/$REPO?ref=main//taskfile.yml
stat git@github.com/$ORGANIZATION/$REPO?ref=main//taskfile.yml: no such file or directory

The following code shows that the org, repo, and file exists ...

$ git clone git@github.com:$ORGANIZATION/$REPO.git --branch main && cp $REPO/taskfile.yml .

@vmaerten
Copy link
Member Author

vmaerten commented Jul 31, 2024

For now, you need to put the schema in front of the URL.
Either ssh://git@github.com/foo/bar.git?ref=main//directory/Taskfile.yml or https://github.com/foo/bar.git?ref=main//directory/Taskfile.yml
I'll impletement later the URL without the scheme

EDIT: like you use the URL for git clone : git@github.com:foo/bar.git?ref=main//directory/Taskfile.yml this works.

@vmaerten
Copy link
Member Author

@blackjid & @pd93 Following this comment : #1652 (comment)
I've changed the url to be go-getter compatible. I've also put the ref optional, default to the default branch

@blackjid
Copy link
Contributor

what do you think about this? #1774
clearly there is need to something...

@vmaerten vmaerten merged commit e6ea064 into go-task:main Sep 24, 2024
14 checks passed
@vmaerten vmaerten deleted the remote-git branch September 24, 2024 17:44
@vmaerten
Copy link
Member Author

@blackjid I've merged this one to move forward on this feature

Let's talk about go-getter or not, in the remote issue or the PR

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.

7 participants