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

Jump to bottom

Add --allow-all flag #1482

Merged
merged 1 commit into from
Jan 9, 2019
Merged

Add --allow-all flag #1482

merged 1 commit into from
Jan 9, 2019

Conversation

ry
Copy link
Member

@ry ry commented Jan 8, 2019

No description provided.

@ry ry requested a review from piscisaureus January 8, 2019 19:34
@piscisaureus
Copy link
Member

Cool no-nonsense name.... You're not wondering if people might get confused and expect it to actually sudo?

kevinkassimo
kevinkassimo reviewed Jan 8, 2019
View reviewed changes
Copy link
Contributor

@kevinkassimo kevinkassimo left a comment

Choose a reason for hiding this comment

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

Personally I'm hesitant about the naming... Might be confusing to some new programmers, especially when we have sudo deno --sudo ...

src/flags.rs Outdated
@@ -26,6 +26,7 @@ pub struct DenoFlags {
pub allow_net: bool,
pub allow_env: bool,
pub allow_run: bool,
pub sudo: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't really need to add this to DenoFlags?...

@ry
Copy link
Member Author

ry commented Jan 8, 2019

Hm - they might...
This looks odd:
sudo deno script.ts --sudo

@ry
Copy link
Member Author

ry commented Jan 8, 2019 •
edited
Loading

--allow
--allow-all
-A
--su
--root

@kevinkassimo
Copy link
Contributor

kevinkassimo commented Jan 8, 2019 •
edited
Loading

-A sounds fine as a short flag (I did propose something similar in the past).
For the long flag I personally prefer --allow-all to match other perm flags (--root --su all have other meanings and might be confusing...)

@piscisaureus
Copy link
Member

--unsafe ?
--godmode
--boss
--unrestricted
--super
--aaa

@kevinkassimo
Copy link
Contributor

--doom (yikes)

@ry
Copy link
Member Author

ry commented Jan 8, 2019

I think --allow-all is most consistent and clear. But --unsafe seems cooler.

@kevinkassimo
Copy link
Contributor

I feel we could reserve --unsafe or --unsafe-* for new flags if we later want to introduce other new flags wrt security model changes?

piscisaureus
piscisaureus approved these changes Jan 8, 2019
View reviewed changes
Copy link
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

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

Lgtm!

kevinkassimo
kevinkassimo approved these changes Jan 8, 2019
View reviewed changes
Copy link
Contributor

@kevinkassimo kevinkassimo left a comment

Choose a reason for hiding this comment

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

LGTM

@shekohex
Copy link

shekohex commented Jan 8, 2019

--yolo 😂

@ry ry changed the title Add --sudo flag Add --allow-all flag Jan 9, 2019
@ry ry force-pushed the sudo branch 2 times, most recently from 0ee3af1 to aae306d Compare January 9, 2019 16:19
@ry ry merged commit 3afdae1 into denoland:master Jan 9, 2019
@ry ry deleted the sudo branch January 9, 2019 17:00
KluVerKamp
KluVerKamp reviewed Jan 17, 2019
View reviewed changes
src/flags.rs
@@ -127,6 +133,7 @@ pub fn set_flags(
opts.optflag("", "allow-net", "Allow network access.");
opts.optflag("", "allow-env", "Allow environment access.");
opts.optflag("", "allow-run", "Allow running subprocesses.");
opts.optflag("A", "allow-all", "Allow all permissions");
Copy link
Contributor

Choose a reason for hiding this comment

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

OCD alert, missing a dot at the end of the line!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KluVerKamp KluVerKamp KluVerKamp left review comments

@piscisaureus piscisaureus piscisaureus approved these changes

@kevinkassimo kevinkassimo kevinkassimo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ry @piscisaureus @kevinkassimo @shekohex @KluVerKamp