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

docker: More convenient access to ipfs commands #3573

Closed
wants to merge 1 commit into from

Conversation

kpcyrd
Copy link
Contributor

@kpcyrd kpcyrd commented Jan 7, 2017

I had to run ipfs repo fsck on my repo inside a docker container, what I had to do:

docker run --entrypoint ipfs myipfs repo fsck

This is because /usr/local/bin/start_ipfs is set as an entrypoint and it explicitly starts ipfs daemon.

This change keeps start_ipfs as an entrypoint (since it's only doing some docker specific checks), but doesn't restrict you in what you can do.

The above command becomes this:

docker run myipfs repo fsck

While, starting the ipfs daemon still looks the same:

docker run myipfs

Bad news (sort of): people already passing arguments to the docker image have to edit their setup from this

docker run myipfs --some-flags

to this:

docker run myipfs daemon --some-flags

I still think this is the better approach since overwriting the entrypoint is sort of the non-obvious way for docker beginners (making the ipfs docker image harder to use for beginners).

License: MIT
Signed-off-by: kpcyrd <git@rxv.cc>
@Kubuxu Kubuxu requested a review from a user January 8, 2017 09:11
@ghost ghost added topic/containers + vms kind/enhancement A net-new feature or improvement to an existing feature labels Jan 8, 2017
@ghost
Copy link

ghost commented Jan 8, 2017

Mh, I generally agree this is useful, but I'd love to see it backward-compatible.

@kpcyrd
Copy link
Contributor Author

kpcyrd commented Jan 9, 2017

I'm not sure what's the sanest way for backwards compatibility, personally I would probably go with a breaking change for the sake of future users.

If it really needs to be reverse compatible, I would test for -- on the first argument and if that's the case, prepend daemon. This would break ipfs --api and ipfs --help (as in, impossible to use without --entrypoint workarounds).

@victorb
Copy link
Member

victorb commented Jan 11, 2017

Yeah, I agree with @kpcyrd, it'll probably be more confusing to be so backward-compatible that it becomes harder to use certain flags.

Makes sense to merge this when we do a minor version bump with other breaking-changes, because this change will in general make a lot of things simpler when interacting with the docker image.

@Kubuxu Kubuxu added this to the ipfs 0.5.0 milestone Jan 11, 2017
@Kubuxu Kubuxu added the status/deferred Conscious decision to pause or backlog label Jan 11, 2017
@whyrusleeping
Copy link
Member

Lets just do this in 0.4.6 and have a bit in the release blog thing about it

@Kubuxu Kubuxu modified the milestones: ipfs 0.4.6, ipfs 0.5.0 Jan 11, 2017
@whyrusleeping
Copy link
Member

Can someone write up something small about the changes in the dockerfile here? @lgierth ?

@ghost
Copy link

ghost commented Feb 12, 2017

I had to run ipfs repo fsck on my repo inside a docker container, what I had to do:

docker run --entrypoint ipfs myipfs repo fsck

This command does work though, so it's merely a matter of convenience. I'm not convinced that's enough to justify breaking backward compatibility.

(Also, I think it's safe to say that the most common operation with the docker image is starting the daemon.)

@ghost
Copy link

ghost commented Feb 12, 2017

I agree the way I went with is unfortunate in retrospect (hardcoding daemon in the entrypoint script instead of setting CMD), but that's what we have now :(

Lets just do this in 0.4.6 and have a bit in the release blog thing about it

That doesn't make it less of a breaking change.

@whyrusleeping
Copy link
Member

@lgierth do you want to close this then?

@kpcyrd
Copy link
Contributor Author

kpcyrd commented Feb 12, 2017

@lgierth the ipfs repo fsck might be a bit abstract, but being able to use ipfs add, ipfs cat and ipfs ls are valid usecases, imo.

The only less breaky way I can think of is changing the script to discard the first argument if it's daemon and deprecate the old syntax. That way people can migrate to the new future proof syntax and old setups don't break until it's removed (in favor of allowing all commands).

kpcyrd added a commit to kpcyrd/go-ipfs that referenced this pull request Feb 12, 2017
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.
kpcyrd added a commit to kpcyrd/go-ipfs that referenced this pull request Feb 12, 2017
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.

Signed-off-by: kpcyrd <git@rxv.cc>
kpcyrd added a commit to kpcyrd/go-ipfs that referenced this pull request Feb 12, 2017
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 <git@rxv.cc>
@whyrusleeping whyrusleeping modified the milestones: Ipfs 0.4.7, ipfs 0.4.6 Feb 14, 2017
@whyrusleeping
Copy link
Member

@kpcyrd @lgierth I merged the other dockerfile PR. Is this one still relevant?

@ghost ghost closed this Mar 2, 2017
@ghost ghost removed the status/deferred Conscious decision to pause or backlog label Mar 2, 2017
@kpcyrd
Copy link
Contributor Author

kpcyrd commented Mar 2, 2017

@whyrusleeping I'm going to keep my branch and reopen this PR a few months after #3685 has been released :)

Thanks everybody.

@whyrusleeping
Copy link
Member

@kpcyrd sounds good to me, thanks!

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 <dgrisham@mines.edu>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature topic/containers + vms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants