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

Add an 'announce' option to announce to custom trackers #427

Merged
merged 1 commit into from Dec 27, 2015

Conversation

5 participants
@valeriangalliat
Contributor

valeriangalliat commented Sep 10, 2015

webtorrent seed somefile -a http://localhost:8000/announce -a udp://localhost:8000 -q

Output:

magnet:?xt=urn:btih:066ed351804835cf0fec21e3b352f58e270a26d5&dn=somefile&tr=http%3A%2F%2Flocalhost%3A8000%2Fannounce&tr=udp%3A%2F%2Flocalhost%3A8000
Add an 'announce' option to announce to custom trackers
    $ webtorrent seed somefile -a http://localhost:8000/announce -a udp://localhost:8000 -q
    magnet:?xt=urn:btih:066ed351804835cf0fec21e3b352f58e270a26d5&dn=somefile&tr=http%3A%2F%2Flocalhost%3A8000%2Fannounce&tr=udp%3A%2F%2Flocalhost%3A8000
@valeriangalliat

This comment has been minimized.

Contributor

valeriangalliat commented Nov 8, 2015

Hey, I would really appreciate a review or any opinion before merging!

@yciabaud

This comment has been minimized.

Contributor

yciabaud commented Nov 8, 2015

I believed magnet-uri was converting tr attributes (like "tr=http%3A%2F%2Flocalhost%3A8000%2Fannounce&tr=udp%3A%2F%2Flocalhost%3A8000") to announce URL list on its own. Am I wrong or is it a convenience argument to achieve the same goal?

@valeriangalliat

This comment has been minimized.

Contributor

valeriangalliat commented Nov 8, 2015

@yciabaud I'm not sure to understand, how would you seed a file from the filesystem with custom announce URLs then?

@yciabaud

This comment has been minimized.

Contributor

yciabaud commented Nov 8, 2015

Oh I may have misunderstood your example. I believed the magnet uri was an argument.

I understand now the purpose of this evolution and I think it is very useful to anyone running a custom tracker.

The PR looks good to me.

@valeriangalliat

This comment has been minimized.

Contributor

valeriangalliat commented Nov 8, 2015

I updated the PR comment to make it more clear, indeed the magnet URI wasn't part of the command.

That's effectively while running a custom tracker I encountered this issue and made the PR, I had to use the JS API to announce on my tracker otherwise.

Thanks for reviewing @yciabaud.

Anything needed before mergin the PR? Like docs somewhere maybe, but I don't see other options documented in the readme except for media output options?

cc @feross

@gillesdemey

This comment has been minimized.

Contributor

gillesdemey commented Nov 8, 2015

Out of curiosity, will this overwrite the existing trackers listed in the torrent file / magnet URI? This may be undesirable if you simple wish to add a tracker on top of the existing ones.

@valeriangalliat

This comment has been minimized.

Contributor

valeriangalliat commented Nov 8, 2015

@gillesdemey Hmm, I didn't consider this before, the idea was just to allow to configure the announce option, whatever this option is doing.

I didn't tried but according to the documentation, it is "added to list in .torrent or magnet uri". 😸

@DiegoRBaquero

This comment has been minimized.

Member

DiegoRBaquero commented Dec 2, 2015

It does overwrite the default trackers. But that's the way create-torrent works atm. If you specify any tracker, at least one, the default are overwritten. https://github.com/feross/create-torrent/blob/master/index.js#L279-L281

We should allow two ways. "announce" and "announceList", as they are already used somehow but both replace. If you use announceList, it overwrites. If you use "announce" it concats them.

What do you think?

@valeriangalliat

This comment has been minimized.

Contributor

valeriangalliat commented Dec 25, 2015

@DiegoRBaquero, I agree it would be nice to have a way to add trackers to the default list instead of overwriting, but in this PR I just want the CLI options to reflect the JS API (so overwriting defaults in any case).

If create-torrent is updated to support a way to append trackers, I'd happily reflect the changes in the CLI options, but modifying create-torrent to support this probably involves quite some discussions and reflection on the right API, and probably better have its place in another PR. :)

@feross

This comment has been minimized.

Member

feross commented Dec 27, 2015

Thanks everyone for the thoughtful discussion. Sorry for taking so long to review this PR. I've been traveling.

The CLI change looks good to me, but we should be consistent and pass the announce option into other commands that support an announce option, like webtorrent create and webtorrent download. I'll make this change after merging.

(FWIW, any collaborator should feel free to merge a PR once there's sufficient support from other collaborators and it's been at least 3 days so everyone has a chance to weigh in with their thoughts. Power to the people 👍)

feross added a commit that referenced this pull request Dec 27, 2015

Merge pull request #427 from valeriangalliat/feature/announce
Add an 'announce' option to announce to custom trackers

@feross feross merged commit 47f9551 into webtorrent:master Dec 27, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@feross

This comment has been minimized.

Member

feross commented Dec 27, 2015

Released as 0.64.0.

@valeriangalliat valeriangalliat deleted the valeriangalliat:feature/announce branch Dec 27, 2015

@lock

This comment has been minimized.

lock bot commented May 5, 2018

This thread has been automatically locked because it has not had recent activity. To discuss futher, please open a new issue.

@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.