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.

Sign up for GitHub

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

Jump to bottom

fix(webrtc): Don't append /p2p to listen addresses #3154

Merged
merged 2 commits into from
Nov 23, 2022
Merged

fix(webrtc): Don't append /p2p to listen addresses #3154

merged 2 commits into from
Nov 23, 2022

Conversation

melekes
Copy link
Contributor

@melekes melekes commented Nov 22, 2022 •
edited by thomaseizinger
Loading

Description

This aligns with what other transports do.

Related: #2622 (comment).

@melekes
[transports/webrtc] remove local peer ID from reported addr
050c416 
to align with all other transports, which don't include `/p2p/..` in their reported addresses.
Refs libp2p#2622 (comment)
@mxinden mxinden added this to the v0.50.0 milestone Nov 22, 2022
@thomaseizinger thomaseizinger changed the title [transports/webrtc] remove local peer ID from reported addr fix(webrtc): Don't append /p2p to listen addresses Nov 22, 2022
thomaseizinger
thomaseizinger reviewed Nov 22, 2022
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks!

I think you have to adjust the tests too to append it there again!

@melekes
Copy link
Contributor Author

melekes commented Nov 23, 2022

The tests are passing for me locally.

CI fails with "Error: No available baseline versions for [email protected]". Not sure what that means.

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Nov 23, 2022 •
edited
Loading

CI fails with "Error: No available baseline versions for [email protected]". Not sure what that means.

That is because of #3141. We will need to adjust the version to actually be greater than what is released.

@thomaseizinger
Copy link
Contributor

The tests are passing for me locally.

Interesting, I thought I added the appending of the /p2p postfix because the tests required dialing with /p2p. Oh well.

@mxinden
Copy link
Member

mxinden commented Nov 23, 2022

It seems like the Testground build of the commits on this pull request fails:

---> Running in d4468d2ff4fa
    Updating git repository `[https://github.com/libp2p/rust-libp2p`](https://github.com/libp2p/rust-libp2p%60)
error: failed to get `libp2p` as a dependency of package `testplan v0.1.0 (/usr/src/testplan/plan)`

Caused by:
  failed to load source for dependency `libp2p`

Caused by:
  Unable to update https://github.com/libp2p/rust-libp2p?rev=9311511a4f50eb4b2ddf6c8d65c9fcba7f81f638

Caused by:
  revspec '9311511a4f50eb4b2ddf6c8d65c9fcba7f81f638' not found; class=Reference (4); code=NotFound (-3)

I am guessing this is due to the fact that this pull request is on a fork and not on the main repository? //CC @thomaseizinger

@mxinden
Copy link
Member

mxinden commented Nov 23, 2022

I have a suspicion:

@melekes
Copy link
Contributor Author

melekes commented Nov 23, 2022

It's a bit weird that build is failing for external contributors 😕

Feel free to cherry-pick this commit! As long as the changes are merged, I'm happy.

mxinden added a commit to mxinden/test-plans that referenced this pull request Nov 23, 2022
@mxinden
ping/rust: Don't set PR ref for master build
e7934db 
When building the `master` image we would previously mistakingly set the
ref to the PR commit hash.

This surfaced when testing a pull request from a fork. `master` would be
pointed at the forks HEAD commit hash, but not the fork URL. Thus the
build could not find the commit and would fail. See
libp2p/rust-libp2p#3154
@mxinden mxinden mentioned this pull request Nov 23, 2022
Merged
mxinden added a commit to libp2p/test-plans that referenced this pull request Nov 23, 2022
@mxinden
fix(ping/rust): Don't set PR ref for master build (#79)
da5bbf7 
When building the `master` image we would previously mistakingly set the
ref to the PR commit hash.

This surfaced when testing a pull request from a fork. `master` would be
pointed at the forks HEAD commit hash, but not the fork URL. Thus the
build could not find the commit and would fail. See
libp2p/rust-libp2p#3154
@mergify mergify bot merged commit babacc5 into libp2p:master Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxinden mxinden mxinden approved these changes

@thomaseizinger thomaseizinger thomaseizinger approved these changes

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
v0.50.0
Development

Successfully merging this pull request may close these issues.
3 participants
@melekes @thomaseizinger @mxinden