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

Implement ipfs shutdown command #3884

Merged
merged 1 commit into from May 18, 2017

Conversation

Projects
None yet
4 participants
@whyrusleeping
Member

whyrusleeping commented Apr 28, 2017

License: MIT
Signed-off-by: Jeromy jeromyj@gmail.com

@kevina

This comment has been minimized.

Contributor

kevina commented Apr 28, 2017

@whyrusleeping is there a reason this is a top-level command:

  ipfs daemon start
  ipfs daemon stop

makes a lot more sense to me, for backwards comparability ipfs daemon can alias to ipfs daemon start

@kevina

I would make sure that this works in offline mode.

return
}
if !nd.OnlineMode() {

This comment has been minimized.

@kevina

kevina Apr 28, 2017

Contributor

I am not 100% sure but I don't think this works when the daemon is started with the --offline flag.

test_expect_success "daemon no longer running" '
test_expect_code 1 kill -0 $IPFS_PID
'

This comment has been minimized.

@kevina

kevina Apr 28, 2017

Contributor

I would also test the daemon when started with the --offline flag.

When I added the test it failed.

@whyrusleeping

This comment has been minimized.

Member

whyrusleeping commented Apr 28, 2017

Its a top level command because its rather difficult to make a command be a subcommand of the daemon command. I tried that at first and it would have required me to start refactoring things.

@whyrusleeping

This comment has been minimized.

Member

whyrusleeping commented Apr 28, 2017

@kevina made it work with --offline. Needed to use the LocalMode check instead of the OnlineMode one

@kevina

kevina approved these changes Apr 28, 2017

This LGTM now.

I would prefer "ipfs daemon stop", but from IRC @whyrusleeping told me that would be difficult due to the special nature of the "daemon" command.

@kevina

This comment has been minimized.

Contributor

kevina commented Apr 28, 2017

BTW @whyrusleeping codeclimate is failing:

exported var DaemonShutdownCmd should have comment or be unexported

@Kubuxu

This comment has been minimized.

Member

Kubuxu commented Apr 28, 2017

Is there any reason to export this command definition.,w:x

@whyrusleeping

This comment has been minimized.

Member

whyrusleeping commented Apr 28, 2017

Yeah, probably no reason to export it

@Kubuxu

Kubuxu approved these changes Apr 28, 2017

@Kubuxu

test failure: need to add polling with timeout for the check that daemon died

Implement ipfs shutdown command
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>

@whyrusleeping whyrusleeping merged commit 630f9ae into master May 18, 2017

6 of 8 checks passed

codecov/patch 20% of diff hit (target 63.93%)
Details
codecov/project 63.92% (-0.01%) compared to 5ef2f42
Details
ci/circleci Your tests passed on CircleCI!
Details
codeclimate no new or fixed issues
Details
commit-message-check/gitcop All commit messages are valid
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@whyrusleeping whyrusleeping deleted the feat/shutdown-cmd branch May 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment