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

Uses environment variables as fallback when no config file is used. #35

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@tutabeier
Contributor

tutabeier commented Aug 8, 2017

In our project we rather set the configurations as environment variables instead of using them hardcoded in a file, following the 12 factor app recommendations for configuration.

This PR is a proposal to use the environment variables as fallback when no config file is been used.

Uses environment variables as fallback when no config file is used.
In our project we rather set the user and password as environment
variables instead of using them hardcoded in a file (following the 12
factor app recommendations). This commit is a propostal to use the
environment variables as fallback when no config file is used.
@@ -160,7 +160,7 @@ In case you want to use client certificates, set the `client_cn` accordingly.
### Dashing Configuration
Edit `config/icinga2.json` and adjust the settings for the Icinga 2 API credentials.
Move `config/icinga2.json.example` to `config/icinga2.json` and adjust the settings for the Icinga 2 API credentials.

This comment has been minimized.

@dnsmichi

dnsmichi Aug 9, 2017

Owner

Mh, I prefer to have a defined configuration in place, although I understand the idea. This should be discussed in a separate issue imho, to move the configuration into a sample.

This comment has been minimized.

@tutabeier

tutabeier Aug 9, 2017

Contributor

If the user follows the documentation and moves the sample file, everything should work as expected, keeping the backward compatibility.

If we don't want to touch the config file right now, we can invert the order and use the config file as fallback in the case we didn't set the env vars. What do you think?

This comment has been minimized.

@dnsmichi

dnsmichi Aug 22, 2017

Owner

Users don't follow the docs, they start right away. That's my main concern.

if [@host, @port, @user, @password].all? {|value| value.nil? or value == ""}
raise ArgumentError.new('No config file or env var found!')
end

This comment has been minimized.

@dnsmichi

dnsmichi Aug 9, 2017

Owner

That looks good to me :)

This comment has been minimized.

@bodsch

bodsch Aug 9, 2017

Contributor

my actual prefer handling are the handling of configuration or environment vars should be located outside of a library.
i think my first step in this library was wrong.

but the code above are okay :)

This comment has been minimized.

@tutabeier

tutabeier Aug 9, 2017

Contributor

I agreed we should move it into a separate file. Do we need to do this now or we can create an issue to understand better how we should implement this?

@dnsmichi

This comment has been minimized.

Owner

dnsmichi commented Jan 20, 2018

I'm going for a different approach here, with a local configuration file being optionally put aside. This works well inside icinga-vagrant already.

I really like the environment variable addition though. I'll implement this in a slightly different patch, making them the default if set. This probably allows for easier Docker containers too, or for local development tests.

Keep in mind, that I'll be using the same variable names as known from the debug console in Icinga 2. This makes it easier for everyone - https://www.icinga.com/docs/icinga2/latest/doc/11-cli-commands/#cli-command-console

In addition to your idea, I'll also add CERT_PATH and NODENAME.

michi@mbmif ~/coding/icinga/dashing-icinga2 (feature/config-env-vars *) $ ICINGA2_API_HOST=localhost ICINGA2_API_PORT=5665 ICINGA2_API_USERNAME=root ICINGA2_API_PASSWORD=icinga dashing start -p 8005
/Users/michi/coding/icinga/dashing-icinga2/binpaths/ruby/2.3.0/gems/thin-1.6.4/lib/thin/backends/base.rb:103: warning: epoll is not supported on this platform
First trying to read environment variables

I'm also going to refactor the code, this needs separate functions called in initialize().

@dnsmichi

This comment has been minimized.

Owner

dnsmichi commented Jan 20, 2018

#52 replaces this PR, your credits on the patch commit have been restored. Thanks for the idea 👍

@dnsmichi dnsmichi closed this Jan 20, 2018

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