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 override config from env variables #8326

Closed
wants to merge 1 commit into from

Conversation

vk496
Copy link

@vk496 vk496 commented Aug 4, 2021

Hello,

It seems a old problem without a clear patch to support it. I've tried to do it as much simple/clean as possible.

With this patch, anyone can override the config with env variable such as:

  • IPFSCONFIG_Swarm_EnableAutoRelay=true
  • IPFSCONFIG_Addresses_Swarm=[\"/ip4/0.0.0.0/tcp/4001\",\"/ip6/::/tcp/4001\"]

Take in account that:

  • Is case sensitive. Is not the same IPFSCONFIG_FooBar than IPFSCONFIG_FOOBAR
  • A valid syntax will be accepted even if the path is wrong (ipfs config stuff). Something like IPFSCONFIG_Swarm_EnableAutoRealy=true will be accepted and written to the config even if it does nothing. So typos could make you loose a lot of hours here :)

Hope it helps you. IPFS is really awesome stuff and now it should be more flexible in container environments

/cc #251 #387 #6325

PS: Docker docs should be updated if is finally merged

@welcome

This comment has been minimized.

vk496 added a commit to vk496/ipfs-docs that referenced this pull request Aug 5, 2021
@aschmahmann
Copy link
Contributor

Is this necessary given that you can just do ipfs daemon --init --init-config <template>?

@vk496
Copy link
Author

vk496 commented Aug 6, 2021

Is this necessary given that you can just do ipfs daemon --init --init-config <template>?

Most people expect use containers as something fungible (or at least, be able to). Since you can't apply new config without restarting the daemon you must use volumes.

I think is very very useful be able start a fresh ipfs container with all desired config with just one cammand and ready to use

@BigLep
Copy link
Contributor

BigLep commented Aug 27, 2021

We think that #8337 is preferred, but we'll dialog about that there.

@lidel
Copy link
Member

lidel commented Sep 3, 2021

This PR was discussed again during GO triage on 2021-09-03, my quick summary:

  • This will work only for a subset of config options
    • For example, there is a hidden complexity in regard to go-ipfs config file syntax around dynamic maps (DNS, pinning service credentials) which makes us worried about the need for domain-specific syntax here – see fix(config): support dynamic map keys with dots #8096. This is not a blocker, but acts as a strong signal this way of setting config will produce bugs and bad ux.
  • Might create confusion
    • This PR adds env variables that work only with Docker, and not standalone go-ipfs.
    • Usually, passing env variables acts as an ad-hoc-one-time override, while this PR makes a permanent change to the config. Restarting container without variable does not restore previous value, which is another papercut which we would like to avoid.
    • If we ever add env variable based config override, it should be supported in go-ipfs natively (go-ipfs/docs/environment-variables.md), and work as "ad-hoc" override that does not mutate the config file.

In short, the decision was to not do this and close this PR without merging, and go with explicit docker hooks proposed in #8337

Thank you for the PoC and pushing the discussion forward @vk496 👍

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