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.

Sign up for GitHub

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 from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


```
vim config/icinga2.json
Expand All @@ -177,8 +177,7 @@ vim config/icinga2.json
}
```

If you don't have any configuration file yet, the default values from the example above
will be used.
If you don't have any configuration file, the environment variables `ICINGA_API_HOST`, `ICINGA_API_PORT`, `ICINGA_API_USER` and `ICINGA_API_PASSWORD` will be used. If these environment variables are not setted, an `ArgumentError` will raise.

If you prefer to use client certificates, set `pki_path` accordingly. The Icinga 2
job expects the certificate file names based on the local FQDN e.g. `pki/icinga2-master1.localdomain.crt`.
Expand Down
0 config/icinga2.json → config/icinga2.json.example
View file
Open in desktop
File renamed without changes.
14 changes: 9 additions & 5 deletions lib/icinga2.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,16 @@ def initialize(configFile)
@password = @config["icinga2"]["api"]["password"]
@pkiPath = @config["icinga2"]["api"]["pki_path"]
else
@log.warn(sprintf('Config file %s not found! Using default config.', configFile))
@host = "localhost"
@port = 5665
@user = "dashing"
@password = "icinga2ondashingr0xx"
@log.warn(sprintf('Config file %s not found. Using ENV vars.', configFile))
@host = ENV['ICINGA_API_HOST']
@port = ENV['ICINGA_API_PORT']
@user = ENV['ICINGA_API_USER']
@password = ENV['ICINGA_API_PASSWORD']
@pkiPath = "pki/"

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

Choose a reason for hiding this comment

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

That looks good to me :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

end

@apiVersion = "v1" # TODO: allow user to configure version?
Expand Down