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

IPNS format keys in b36cid #7554

Merged
merged 3 commits into from
Aug 4, 2020
Merged

IPNS format keys in b36cid #7554

merged 3 commits into from
Aug 4, 2020

Conversation

petar
Copy link
Contributor

@petar petar commented Jul 20, 2020

No description provided.

@petar petar requested a review from aschmahmann July 20, 2020 17:30
@petar
Copy link
Contributor Author

petar commented Jul 20, 2020

To run sharness locally, during dev, run this from the test/sharness dir:

TEST_NO_FUSE=1 GOIPFSTEST_SKIP_LOCAL_DEVTREE_DEPS_CHECK=1 make

Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

We need more sharness tests here. In particular we should

  1. Like with the previous PR, have a function that will let us test that Ed25519 + RSA keys both work (it's not quite in line with this PR but we need to do it anyway
  2. Make sure that we can publish using a b58mh as well as a b36cid. The results can all come out as Publish to ${b36cidKey} though.

core/coreapi/name.go Outdated Show resolved Hide resolved
core/coreapi/name.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
test/sharness/lib/test-lib.sh Outdated Show resolved Hide resolved
core/commands/keystore.go Outdated Show resolved Hide resolved
@petar
Copy link
Contributor Author

petar commented Jul 21, 2020

We need more sharness tests here. In particular we should

  1. Like with the previous PR, have a function that will let us test that Ed25519 + RSA keys both work (it's not quite in line with this PR but we need to do it anyway
  2. Make sure that we can publish using a b58mh as well as a b36cid. The results can all come out as Publish to ${b36cidKey} though.

I've added a test for (2). For (1), it seems one good strategy is to wrap all tests in a function parameterized by ed25519/rsa and use this to initialize the self ID, because most tests are based on the self id. Would you prefer a different approach?

@aschmahmann
Copy link
Contributor

(1), it seems one good strategy is to wrap all tests in a function parameterized by ed25519/rsa and use this to initialize the self ID, because most tests are based on the self id. Would you prefer a different approach?

We need to test publishing with the --key flag anyway. So maybe something like this:

  • Have a function that tests with ed25519, rsa, and self
  • Test that publishing with no --key flag means we published with self
  • Test that we can publish with --key=peerIDRepresentationOfKey, or --key=friendlyName

@petar
Copy link
Contributor Author

petar commented Jul 21, 2020

(1), it seems one good strategy is to wrap all tests in a function parameterized by ed25519/rsa and use this to initialize the self ID, because most tests are based on the self id. Would you prefer a different approach?

We need to test publishing with the --key flag anyway. So maybe something like this:

  • Have a function that tests with ed25519, rsa, and self
  • Test that publishing with no --key flag means we published with self
  • Test that we can publish with --key=peerIDRepresentationOfKey, or --key=friendlyName

I think now all these cases are covered.

test/sharness/t0100-name.sh Outdated Show resolved Hide resolved
test/sharness/t0100-name.sh Outdated Show resolved Hide resolved
test/sharness/t0100-name.sh Outdated Show resolved Hide resolved
test/sharness/t0100-name.sh Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
test/sharness/t0100-name.sh Outdated Show resolved Hide resolved
test/sharness/t0100-name.sh Outdated Show resolved Hide resolved
test/sharness/t0100-name.sh Show resolved Hide resolved
Comment on lines 233 to 243
test_expect_success "generate and verify a new key" '
B58MH_ID=`ipfs key list -f=b58mh -l | grep self | cut -d " " -f1` &&
B36CID_ID=`ipfs key list -f=b36cid -l | grep self | cut -d " " -f1` &&
test_check_peerid "${B58MH_ID}" &&
test_check_peerid "${B36CID_ID}"
'

test_expect_success "'ipfs name publis --allow-offline --key=<peer-id> <hash>' succeeds" '
ipfs name publish --allow-offline --key=${B58MH_ID} "/ipfs/$HASH_WELCOME_DOCS" >b58mh_published_id &&
ipfs name publish --allow-offline --key=${B36CID_ID} "/ipfs/$HASH_WELCOME_DOCS" >b36cid_published_id
'
Copy link
Contributor

Choose a reason for hiding this comment

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

This code tests publishing with the self key which what is tested in the other function test_name_with_self . Did you mean to use the generated key here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No: This test is focused on using an explicit key. Moved it to test_name_with_self.

test/sharness/t0100-name.sh Show resolved Hide resolved
core/commands/fmt.go Outdated Show resolved Hide resolved
@aschmahmann aschmahmann merged commit 18a2756 into master Aug 4, 2020
@aschmahmann aschmahmann mentioned this pull request Sep 22, 2020
72 tasks
@hacdias hacdias deleted the petar/namefmt branch May 9, 2023 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants