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

feat: log permission access #2518

Merged
merged 9 commits into from Jun 22, 2019

Conversation

4 participants
@bartlomieju
Copy link
Contributor

commented Jun 14, 2019

Closes #2514

Example output:

$ ./target/debug/deno --allow-net --allow-read https://deno.land/std/http/file_server.ts
ℹ️  Granted network access to "0.0.0.0:4500"
HTTP server listening on http://0.0.0.0:4500/
ℹ️  Granted read access to "/Users/biwanczuk/dev/deno"
ℹ️  Granted read access to "/Users/biwanczuk/dev/deno"

$ ./target/debug/deno --allow-net --allow-read --no-prompt https://deno.land/std/http/file_server.ts
HTTP server listening on http://0.0.0.0:4500/

Screenshot for reference:
Zrzut ekranu 2019-06-14 o 16 13 19

I'm open to @kitsonk's suggestion to introduce another level of internal logging: now we have -D/--log-debug, how about we introduce -I/--log-info?

@hayd

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

how about we introduce -I/--log-info?

👍

@kitsonk

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

Or how about we introduce --log-level=debug or something that means we are not proliferating a load of flags.

@bartlomieju bartlomieju force-pushed the bartlomieju:feat-log_perm_access branch from 6cfd2e7 to 56269a5 Jun 15, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

Or how about we introduce --log-level=debug or something that means we are not proliferating a load of flags.

Done! Added --log-level/-L flag that takes either info or debug value.
(I don't really like -L 🙄... let you be the judge)

I kept -D/--log-debug flag for compatibility - basically it's an alias for --log-level=debug.

Now permission access log is shown only with --log-level=info or --log-level=debug:

deno_dev --log-level=info -A https://deno.land/std/http/file_server.ts
ℹ️  Granted network access to "0.0.0.0:4500"
HTTP server listening on http://0.0.0.0:4500/
ℹ️  Granted read access to "/Users/biwanczuk/dev/deno"
ℹ️  Granted read access to "/Users/biwanczuk/dev/deno"
ℹ️  Granted read access to "/Users/biwanczuk/dev/deno/Cargo.toml"
ℹ️  Granted read access to "/Users/biwanczuk/dev/deno/buildtools"
ℹ️  Granted read access to "/Users/biwanczuk/dev/deno/tools"
...
@ry

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2019

The naming of these type of flags has been done before. We'd probably be best off by using existing nomenclature.

In glog ( https://godoc.org/github.com/golang/glog ) they use -v=$LEVEL ... I'm most familiar with glog flags like -logtostderr... But I know many people hate glog. Does anyone have alternative logging systems we could steal ideas from?

@kitsonk

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

Previous art:

  • Rust is an environment variable RUST_LOG so you get RUST_LOG=info
  • Python is --log so --log=INFO
  • npm uses --loglevel so you get --loglevel verbose but also has individual flags to control things as well.

There were a few other examples, but the best thing is that they are totally inconsistent. Some have just simple flags, some only have environment variables. 🤷‍♂ It does seem like DENO_LOG could/should be respected though.

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

In glog ( https://godoc.org/github.com/golang/glog ) they use -v=$LEVEL

I like this approach - -v/--verbosity seems very intuitive.

By default we log out only error messages, setting -v=DEBUG would be equivalent of current -D, while -v=INFO would show permission prompts.

I also agree with @kitsonk on handling DENO_LOG env variable as well.

Show resolved Hide resolved cli/flags.rs Outdated
Show resolved Hide resolved cli/permissions.rs Outdated
@ry
Copy link
Collaborator

left a comment

Sorry for the slow review. Looks great. I think we can bike-shed on the naming of the flag for a while longer and just land this one now. I have just a few comments regarding the emoji and whether we should remove -D...

@ry

ry approved these changes Jun 22, 2019

Copy link
Collaborator

left a comment

LGTM

@ry ry merged commit b9fbd55 into denoland:master Jun 22, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@bartlomieju bartlomieju deleted the bartlomieju:feat-log_perm_access branch Jun 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.