daemon: use fsrepo.IsInitialized to test for initialization #3805
Conversation
test/sharness/t0063-daemon-init.sh
Outdated
| test_ipfs_daemon_init | ||
|
|
||
| test_expect_success "create empty \$IPFS_PATH dir" ' | ||
| rm -rf "$IPFS_PATH" |
There was a problem hiding this comment.
best to put && after the rm, I know it "shouldn't" fail but still.
There was a problem hiding this comment.
Sorry. The "&&" are too easy to forget about.
kevina
force-pushed
the
kevina/repo-init
branch
from
7b09b14
to
99f628f
Compare
cmd/ipfs/daemon.go
Outdated
| // `IsInitialized` where the quality of the signal can be improved over | ||
| // time, and many call-sites can benefit. | ||
| if !util.FileExists(req.InvocContext().ConfigRoot) { | ||
| if !fsrepo.IsInitialized(req.InvocContext().ConfigRoot) { |
There was a problem hiding this comment.
Instead of calling twice the req.InvocContex().ConfigRoot why not make under initialize something like
if initialize {
cfg := req.InvocContext()
if !fsrepo.IsInitialized(cfg) {
err := initWithDefaults(os.Stdou, cfg)
// code
kevina
force-pushed
the
kevina/repo-init
branch
from
99f628f
to
ddd4aef
Compare
|
@whyrusleeping okay done |
| # server. | ||
|
|
||
| test_expect_success "'ipfs daemon --init' succeeds" ' | ||
| ipfs daemon --init >actual_daemon 2>daemon_err & |
There was a problem hiding this comment.
This will be problematic with other daemons people have running (port numbers). Not really sure how to solve it.
There was a problem hiding this comment.
Yeah... i'm not sure how to fix it either. We should look into adding a way to specify ports from here.
Though not sure if this should block the PR, Its probably a separate concern
There was a problem hiding this comment.
It will cause the test to fail on users machines who also have an IPFS daemon running. Should we flag it some how? If so I am not sure how.
There was a problem hiding this comment.
We could check if ports from config are bound (8080, 4001, 5001) using netcat and if any of those are, skip the test. How that sounds?
There was a problem hiding this comment.
Yeah, lets do that. Its better to print a message about why it fails rather than just randomly failing.
There was a problem hiding this comment.
You should be able to use nc -z -w1 localhost 5001
There was a problem hiding this comment.
Do you still want to do that? The test won't fail currently.
There was a problem hiding this comment.
@kevina Hrm... i think we can probably do without for now. Lets add a todo to add a flag for different ports on the daemon
There was a problem hiding this comment.
@whyrusleeping do you think we I should reduce the timeout to 1 sec to minimize the chance of this test failing?
There was a problem hiding this comment.
I wouldnt worry about it, lets just make a note to do proper port selection for this later.
Use fsrepo.IsInitialized to test for initialization instead of just testing if the directory exists. Also add test cases for '--init' option, including one the creates an empty directory. License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
kevina
force-pushed
the
kevina/repo-init
branch
from
ddd4aef
to
2892147
Compare
|
I added a note as an issue. This should be good to go... |
Use fsrepo.IsInitialized to test for initialization instead of just testing if the directory exists.
Also add test cases for '--init' option, including one the creates an empty directory.
Closes #3773.