Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Move to not using multiaddr for config values #791

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

asutula
Copy link
Member

@asutula asutula commented Mar 3, 2021

Per a conversation today with @sanderpick and @textileben . So far, I've only update the lotus address to be plain old host:port, but we can take this all the way. Let me know what you think.

Signed-off-by: Aaron Sutula <hi@asutula.com>
@asutula asutula requested a review from jsign March 3, 2021 00:09
@asutula asutula self-assigned this Mar 3, 2021
Copy link
Contributor

@jsign jsign left a comment

Choose a reason for hiding this comment

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

Sounds good.

But maybe we should hold merging this until a next major version bump since this in theory is a breaking change. Not in the APIs, but in the flag/envs that people might already have and are forced to change.

@@ -32,7 +32,7 @@ func defaultServerConfig(t *testing.T) server.Config {
ipfsAddr := util.MustParseAddr(ipfsAddrStr)

devnet := tests.LaunchDevnetDocker(t, 1, 300, ipfsAddrStr, false)
devnetAddr := util.MustParseAddr("/ip4/127.0.0.1/tcp/" + devnet.GetPort("7777/tcp"))
devnetAddr := "127.0.0.1/tcp/" + devnet.GetPort("7777/tcp")
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this should be:
devnetAddr := "127.0.0.1:" + devnet.GetPort("7777/tcp")

Copy link
Member Author

Choose a reason for hiding this comment

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

👍🏼

Signed-off-by: Aaron Sutula <hi@asutula.com>
@asutula
Copy link
Member Author

asutula commented Mar 3, 2021

@jsign I realized there were other places, like docs strings and readme that needed to be updated. May want to take another look. Sure, we can keep this on a branch for now. But once this is used in Hub, and we need to deploy hub (still might be many days or a week or two out), we'll have to release this.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants