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

CONFIG_FILE environment variable should be configurable #851

Closed
cwjohnston opened this issue Nov 28, 2017 · 8 comments · Fixed by #861
Closed

CONFIG_FILE environment variable should be configurable #851

cwjohnston opened this issue Nov 28, 2017 · 8 comments · Fixed by #861

Comments

@cwjohnston
Copy link
Contributor

cwjohnston commented Nov 28, 2017

Description of problem

The value of CONFIG_FILE environment variable is not currently configurable via this module.

Both Sensu Core and Sensu Enterprise use the value of the CONFIG_FILE environment variable to set the --config flag when starting Sensu processes. This flag specifies a singular JSON config file, the content of which is deep merged with the contents of the directory specified by the --config_dir flag. The CONFD_DIR environment variable used to set the value of --config_dir flag is already configurable via this module.

The default CONFIG_FILE value is /etc/sensu/config.json (e.g. https://github.com/sensu/sensu-omnibus/blob/master/files/sensu-gem/bin/sensu-service#L46-L55)

@ghoneycutt
Copy link
Collaborator

I do not see CONFIG_FILE listed in #849
Seems like that should be configurable in /etc/default/sensu, correct?

@cwjohnston
Copy link
Contributor Author

@ghoneycutt yes, CONFIG_FILE is honored by both Sensu Core and Sensu Enterprise, so I believe /etc/default/sensu is the right place.

@alvagante
Copy link
Collaborator

alvagante commented Nov 29, 2017

@cwjohnston @ghoneycutt If you agree I'd add it with eventually all the others listed in #849.
Note that given the current code base this would require adding relevant parameters to the main class in init.pp, and at least packages.pp. There parameters would match environment variables like SENSU_CLIENT_SUBSCRIPTIONS, SENSU_TRANSPORT_NAME, RABBITMQ_URL, REDIS_URL which are already defined in other parameters.
In order to manage CONFIG_FILE, the above environment variables, and anything that is used in /etc/default/sensu (and, eventually with a different parameter, in /etc/default/sensu-enterprise), I would introduce a single parameter, an hash of configuration settings, so that there will no need in the future to introduce new variables to support new settings for those files.
In order to preserve compatibilty with current parameters, I'd merge the user provided hash, in this new parameter (something like env_vars) with a default one, with settings based on the current parameters of the class.
I probably take more time describing than doing, anyway.
If you want I can prepare the code that implements this to give you an idea and then decide if to follow it. Scope is the configuration of /etc/default/sensu and /etc/default/sensu-enterprise and the relevant current and future parameters to manage them.

@ghoneycutt
Copy link
Collaborator

I like this approach though it will require a major version change if we change parameters.

@cwjohnston
Copy link
Contributor Author

Please see my note in #849 (comment). I am not in favor of managing env vars that overlap with config already managed by JSON.

@alvagante
Copy link
Collaborator

@ghoneycutt actually we can reserve the existing parameters, eventually with a deprecation notice of the ones we want to get rid of.

@cwjohnston as in the note of #849 (comment) we can use this hash based approach for the settings of environment variables which refer to elements not present in the json configs.

Actually we can allow the user also to set variables like REDIS_URL if he really wanted them in /etc/default/sensu . SO we can achieve both compatibility with current params and full customisability from users, for any current or future setting.
going to prepare a sample pr (code only , tests later) to show you the idea, I'm quite sure you'll like it.

alvagante added a commit to alvagante/sensu-puppet that referenced this issue Nov 29, 2017
alvagante added a commit to alvagante/sensu-puppet that referenced this issue Dec 22, 2017
@alvagante
Copy link
Collaborator

alvagante commented Dec 22, 2017

@ghoneycutt #861 does the job and replaces #852
@cwjohnston sorry for taking some much time on this, the alternative approach I tried earlier was not worth the effort.

ghoneycutt added a commit that referenced this issue Jan 4, 2018
Added config_file params to CONFIG_FILE envvar #851
@ghoneycutt
Copy link
Collaborator

Released in v2.43.0

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 a pull request may close this issue.

3 participants