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

daemon: use fsrepo.IsInitialized to test for initialization #3805

Merged
merged 1 commit into from
Mar 23, 2017

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Mar 20, 2017

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.

@kevina kevina added the status/in-progress In progress label Mar 20, 2017
@kevina kevina added this to the Ipfs 0.4.8 milestone Mar 20, 2017
test_ipfs_daemon_init

test_expect_success "create empty \$IPFS_PATH dir" '
rm -rf "$IPFS_PATH"
Copy link
Member

Choose a reason for hiding this comment

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

best to put && after the rm, I know it "shouldn't" fail but still.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. The "&&" are too easy to forget about.

// `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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Address the other comment here then LGTM.

@kevina
Copy link
Contributor Author

kevina commented Mar 21, 2017

@whyrusleeping okay done

# server.

test_expect_success "'ipfs daemon --init' succeeds" '
ipfs daemon --init >actual_daemon 2>daemon_err &
Copy link
Member

Choose a reason for hiding this comment

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

This will be problematic with other daemons people have running (port numbers). Not really sure how to solve it.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, lets do that. Its better to print a message about why it fails rather than just randomly failing.

Copy link
Member

Choose a reason for hiding this comment

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

You should be able to use nc -z -w1 localhost 5001

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you still want to do that? The test won't fail currently.

Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@whyrusleeping do you think we I should reduce the timeout to 1 sec to minimize the chance of this test failing?

Copy link
Member

Choose a reason for hiding this comment

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

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 <k@kevina.org>
@kevina
Copy link
Contributor Author

kevina commented Mar 23, 2017

I added a note as an issue. This should be good to go...

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @kevina!

@whyrusleeping whyrusleeping merged commit a3fc6f1 into master Mar 23, 2017
@whyrusleeping whyrusleeping deleted the kevina/repo-init branch March 23, 2017 03:55
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.

4 participants