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

more loglevel info #152

Closed
wants to merge 3 commits into from
Closed

more loglevel info #152

wants to merge 3 commits into from

Conversation

markhu
Copy link

@markhu markhu commented Oct 24, 2018

adds log.Info to address issue #151

adds log.Info to address issue #151
@markhu markhu changed the title more loglevel info per issue #151 more loglevel info Oct 24, 2018
@codecov-io
Copy link

codecov-io commented Oct 24, 2018

Codecov Report

Merging #152 into master will increase coverage by 0.15%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #152      +/-   ##
==========================================
+ Coverage   36.16%   36.32%   +0.15%     
==========================================
  Files          10       10              
  Lines         683      680       -3     
==========================================
  Hits          247      247              
+ Misses        397      394       -3     
  Partials       39       39
Impacted Files Coverage Δ
cli/add.go 17.55% <0%> (+0.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c709c2...0a99bc1. Read the comment docs.

Copy link
Contributor

@godrei godrei left a comment

Choose a reason for hiding this comment

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

i would prefer to notify the user with a single line of message, like:

log.Infof("[ENVMAN] added %s: %s", key, value)

@viktorbenei
Copy link
Contributor

Awesome idea @markhu ! Don't forget to address @godrei 's change request ;)

@@ -213,6 +213,8 @@ func add(c *cli.Context) error {

if err := addEnv(key, value, expand, replace, skipIfEmpty); err != nil {
log.Fatal("[ENVMAN] Failed to add env:", err)
} else {
log.Infof("[ENVMAN] added %s: %s", key, value)
Copy link
Author

Choose a reason for hiding this comment

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

A single log line.

Copy link
Author

@markhu markhu left a comment

Choose a reason for hiding this comment

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

Done.

@trapacska
Copy link
Contributor

Hey @markhu 👋

Just dig myself into this change and I have couple of concerns about it.

  • When you export a key:value pair for example bash, you won't have confirmation message about the successful state, could you please describe why is it required to be printed at info log level?
  • In the add subcommand we don't print anything at info level, having a new log level type wired in the add subcommand would mean a breaking change for those who rely on the log to be empty in case of success.
  • It can also cause multiple security issues as well, in couple of cases users are exporting their secret values into ENVs by envman, exposing these values are not acceptable in the default log level(which is info), it should take separated action to log these out. (I suggest it to be a debug log type)

Thanks for your response!
🚀

@markhu
Copy link
Author

markhu commented Dec 10, 2018

The info log level is not normally visible. So the default behavior addresses all your concerns.

@trapacska
Copy link
Contributor

The default log level is info. 🙂
screen shot 2018-12-11 at 9 20 43 am

@trapacska
Copy link
Contributor

@markhu So could you please move this log into a higher log level?

@trapacska
Copy link
Contributor

Hey @markhu !

Closing the issue as we didn't receive response about the review. Feel free to re-open or contact us any time of you want to move forward with it! 🙂

Happy Building! 🚀

@trapacska trapacska closed this Jan 7, 2019
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.

5 participants