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

Suppressing logging of configuration files on init #237

Merged
merged 2 commits into from
Apr 12, 2017
Merged

Suppressing logging of configuration files on init #237

merged 2 commits into from
Apr 12, 2017

Conversation

sigv
Copy link
Contributor

@sigv sigv commented Apr 6, 2017

Right now subcommand.rb logs something along the following on start-up if a configuration file exists:

[hiera-eyaml-core] Loaded config from /home/user/.eyaml/config.yaml

That kind of debugging information is understandable and can be helpful for seeing how your configuration is applied, but it is very bad for non-interactive use. Additionally, the behavior for loading these configuration files is documented in the README, so I am proposing removing those logs for the benefit of being able to properly script eyaml encrypt as well as eyaml decrypt.

@sigv
Copy link
Contributor Author

sigv commented Apr 6, 2017

Tests breakage has been occurring already for Ruby 1.8.7.

@sigv
Copy link
Contributor Author

sigv commented Apr 6, 2017

And yes, that logging is output on stderr so you can already do shell scripting, but there is that logging output that you can't really get rid of without my proposed change (or possibly rewriting the way the CLI arguments are parsed, so that parsing takes place before the logging call). I do not want to have to forward all stderr to /dev/null since I would like to be able to actually see errors if they occur.

$ eyaml encrypt -s Hello -o string | eyaml decrypt --stdin
[hiera-eyaml-core] Loaded config from /home/user/.eyaml/config.yaml
[hiera-eyaml-core] Loaded config from /home/user/.eyaml/config.yaml
Hello

@rnelson0
Copy link
Member

rnelson0 commented Apr 7, 2017

@sigv Yeah, we really need to fix the test failures, sorry :( But we aren't holding up PRs on them at least.

Is there a way to write that info out only when debugs are enabled? I would guess something like LoggingHelper::debug or ::trace but I'm not that familiar enoughwith this program yet to say which.

@sigv
Copy link
Contributor Author

sigv commented Apr 7, 2017

I could switch it over, but the parse function which reads in the arguments is handled only after that logging call. Are you suggesting it would be better that I refactor the codebase so that the parsing of the arguments takes place before logging about configuration files?

@sigv
Copy link
Contributor Author

sigv commented Apr 7, 2017

That is, if I just switch the info level out for debug right now as-is, then that logging call will never actually print anything even if the verbosity is set for debugging.

@rnelson0
Copy link
Member

rnelson0 commented Apr 7, 2017

Well, poop. It does seem helpful to print the name of the file used when in debug mode, If you move it down a line, after the config is read, would that help? Technically it shouldn't say Loaded until after the load is complete, anyway.

@rnelson0
Copy link
Member

rnelson0 commented Apr 7, 2017

A quick glance suggest that it probably isn't that easy. If you can find a way to preserve this info somehow, great, but if not I think it's beneficial to merge as is so that silent operation is actually silent. Let me know.

@sigv
Copy link
Contributor Author

sigv commented Apr 7, 2017 via email

sigv added 2 commits April 7, 2017 18:18
Up until now, you could not directly use `eyaml` in shell scripting
along with a configuration file since each such invocation resulted
in additional debugging logs being included in the output. Those logs
can be dropped since they are informing the user about well documented
behavior (see README) for the sake of allowing scripting encyption
and decryption in a simple non-interactive fashion with plain pipes.

> [hiera-eyaml-core] Loaded config from /home/user/.eyaml/config.yaml
We do want to log where our configuration comes from as mentioned
in #237 but that should take place on the `debug`
level and not the `info` level, since the behavior is documented in
the README and should only appear in verbose logs.

Additionally, that kind of logging should take place after all
the options have been parsed, since it matters what kind of verbosity
users want to have output.. and we do not know that until after
parsing. That results in a little bit of messy code where the sources
of the options are passed along with the options themselves, but also
means possible expansion in the future. Maybe.
@sigv
Copy link
Contributor Author

sigv commented Apr 7, 2017

Tests seem to be passing locally for me. Behavior wise, this like an acceptable change to me - only output where the configuration is being loaded from, when the user requests additional verbosity.

Example demo log output:

$ eyaml version
[hiera-eyaml-core] hiera-eyaml (core): 2.1.0
[hiera-eyaml-core] hiera-eyaml-plaintext (gem): 0.6
$ eyaml version -v
[hiera-eyaml-core] Loaded config from /home/user/.eyaml/config.yaml
[hiera-eyaml-core] hiera-eyaml (core): 2.1.0
[hiera-eyaml-core] hiera-eyaml-plaintext (gem): 0.6

I would however appreciate comments on my Ruby code styling since I am not really used to writing Ruby. 😄

@rnelson0
Copy link
Member

rnelson0 commented Apr 7, 2017

Seems Ruby-ish to me. Thanks!

@sigv
Copy link
Contributor Author

sigv commented Apr 12, 2017

Any plans on reviewing and eventually merging this change? 🤔

@rnelson0 rnelson0 merged commit 87914c6 into voxpupuli:master Apr 12, 2017
@rnelson0
Copy link
Member

@sigv Sorry! I think I was on my phone when I reviewed this and forgot to hit merge. Thanks!

@sigv sigv deleted the drop-configuration-info branch April 12, 2017 19:52
ccojocar pushed a commit to ccojocar/hiera-eyaml that referenced this pull request Jul 10, 2017
We do want to log where our configuration comes from as mentioned
in voxpupuli#237 but that should take place on the `debug`
level and not the `info` level, since the behavior is documented in
the README and should only appear in verbose logs.

Additionally, that kind of logging should take place after all
the options have been parsed, since it matters what kind of verbosity
users want to have output.. and we do not know that until after
parsing. That results in a little bit of messy code where the sources
of the options are passed along with the options themselves, but also
means possible expansion in the future. Maybe.
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.

2 participants