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

Dockerfile: Deprecate implicit daemon argument #3685

Merged
merged 3 commits into from
Mar 2, 2017

Conversation

kpcyrd
Copy link
Contributor

@kpcyrd kpcyrd commented Feb 12, 2017

After the discussion in #3573 this patch prints a deprecation warning if:

  1. the image has been executed with additional arguments
  2. the first argument isn't daemon

This way people are able to migrate to the new syntax without any breaking changes.

License: MIT
Signed-off-by: kpcyrd [email protected]

@kpcyrd kpcyrd force-pushed the dockerfile_prepare_cmd branch 2 times, most recently from 3588cbb to 77e4c64 Compare February 12, 2017 19:52
echo "DEPRECATED: run 'docker run ipfs/go-ipfs daemon $@' instead" >&2
echo "DEPRECATED: see the following PRs for more information:" >&2
echo "DEPRECATED: * https://github.com/ipfs/go-ipfs/pull/3573" >&2
echo "DEPRECATED: * https://github.com/ipfs/go-ipfs/pull/3685" >&2
Copy link
Member

Choose a reason for hiding this comment

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

What would be nice is to have something like tracking pixel to know when people made transition but some might deem it to intrusive.

After the discussion in ipfs#3573 this patch prints a deprecation warning if:

1) the image has been executed with additional arguments
2) the first argument isn't daemon

This way people are able to migrate to the new syntax without any breaking changes.

License: MIT
Signed-off-by: kpcyrd <[email protected]>
License: MIT
Signed-off-by: kpcyrd <[email protected]>
This keeps docker containers from entering a crashloop on new image versions

License: MIT
Signed-off-by: kpcyrd <[email protected]>
@ghost ghost added this to the Ipfs 0.4.7 milestone Feb 23, 2017
@ghost
Copy link

ghost commented Feb 23, 2017

This LGTM, thanks! 👍

Apart from the daemon argument, this also makes --migrate=true the default, which is reasonable. The default arguments of the container should not lead to a situation where it hangs waiting for stdin (y/n).

@kpcyrd
Copy link
Contributor Author

kpcyrd commented Feb 23, 2017

Apart from the daemon argument, this also makes --migrate=true the default, which is reasonable. The default arguments of the container should not lead to a situation where it hangs waiting for stdin (y/n).

yup, sorry for the unrelated changes, but that's exactly what happened for me :)

@whyrusleeping whyrusleeping merged commit c8d63ec into ipfs:master Mar 2, 2017
@kpcyrd kpcyrd deleted the dockerfile_prepare_cmd branch March 2, 2017 15:32
dgrisham added a commit to ipfs/iptb that referenced this pull request Sep 10, 2018
This commit adds `daemon` to the end of the `docker run` args whenever the user
passes in `<ipfs-args>` to `iptb start -- <ipfs-args>`. Without this additional
argument, the IPFS daemon logs a deprecation warning. We don't want to pass
`daemon` into `docker run` when the user hasn't provided additional args,
because `docker run` has default args when nothing else is passed.

For more info, see:

-   ipfs/kubo#3573
-   ipfs/kubo#3685

License: MIT
Signed-off-by: David Grisham <[email protected]>
@badkk
Copy link

badkk commented Apr 4, 2019

what if I want run ipfs bootstrap rm --all before ipfs daemon

@kpcyrd
Copy link
Contributor Author

kpcyrd commented Apr 5, 2019

@LucasDvP this patch was preparing a setup that would support something like this.

I've filed #6188

After that patch has been merged you can do:

docker run --rm -v /some/folder:/data/ipfs ipfs/go-ipfs bootstrap rm --all
docker run --rm -v /some/folder:/data/ipfs ipfs/go-ipfs

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

Successfully merging this pull request may close these issues.

4 participants