handshake-org / hsd

Handshake Daemon & Full Node
Other
1.9k stars 273 forks source link

Fix typo in Docker example #890

Closed sethforprivacy closed 2 months ago

sethforprivacy commented 2 months ago

This typo breaks the Docker command, as hsd is expecting an =.

Falci commented 2 months ago

I have a server running it with --http-host '::' and it works fine.

sethforprivacy commented 2 months ago

Running the image here, hsd crashed on startup claiming that the http-host arg didn't exist due to the space instead of =.

As soon as I made it match every other config value is supposed to be passed in it started right up without issues.

Falci commented 2 months ago

Tested again with the following code:

docker run --name hsd \
    --publish 12037:12037 \
    --volume /tmp/hsd:/root/.hsd \
    ghcr.io/handshake-org/hsd:latest \
    --http-host 0.0.0.0 \
    --api-key=foo

And it worked with or without the =.

Also, checked bcfg and it seems to support both space or =.

There's no harm in merging this PR, but I don't see how this could be root of the problem you faced, unless there's a bigger issue with bcfg.

sethforprivacy commented 2 months ago

Ah, perhaps it's a difference in how Docker Compose's command arg parses these?

Will share the format I used shortly that had issues.

Edit: Here is the format I used that errored out:

  hsd:
    image: ghcr.io/handshake-org/hsd:latest
    restart: unless-stopped
    container_name: hsd
    volumes:
      - hsd-data:/root/.hsd
    ports:
      - 127.0.0.1:12037:12037
      - 12038:12038
      - 127.0.0.1:12039:12039
      - 44806:44806
    command:
      - "--http-host 0.0.0.0"
      - "--api-key=foo"
rithvikvibhu commented 2 months ago

I think similar to Dockerfile CMD, the command, an array, should be split and separated by spaces: https://docs.docker.com/reference/dockerfile/#cmd

So if you want to use spaces in the compose file, then it should look like

commands:
    - "--http-host"
    - "0.0.0.0"
    - "--api-key"
    - "foo"

Or of course, like the previous comment, using = in a single line will work since it's still 1 argument passed to hsd/bcfg.

This behaviour applies to any executable/program receiving arguments, not just hsd/bcfg.

Since the current example in INSTALL.md is with docker CLI (and not Compose), I don't think it's wrong; space is perfectly acceptable.

However, adding a new Compose example below this one would be helpful. It can use = which brings the keys and values on the same line making it easier to read.

sethforprivacy commented 2 months ago

Good catch, @rithvikvibhu, has been so long since I used docker run instead of Compose I had forgotten that difference.

Will open a PR adding an example instead!

sethforprivacy commented 2 months ago

Related PR: https://github.com/handshake-org/hsd/pull/891