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

Change default value of --disable-sandbox #1719

Merged
merged 1 commit into from Nov 6, 2020

Conversation

fthomas
Copy link
Member

@fthomas fthomas commented Nov 6, 2020

This changes the default value of the --disable-sandbox option from
false to true. I think this is a better default since sandboxed
execution is only useful when the operator of Scala Steward does not
trusts the repositories it is working on. This is only true for public
instances and (almost) never true for private instances. And there are
a lot more private than public instances.

Other arguments for this change are that firejail does not work inside
Docker and that documentation and setup become simpler.

To enable sandboxed execution, --disable-sandbox=false or the newly
added --enable-sandbox option can be used.

This changes the default value of the `--disable-sandbox` option from
`false` to `true`. I think this is a better default since sandboxed
execution is only useful when the operator of Scala Steward does not
trusts the repositories it is working on. This is only true for public
instances and (almost) never true for private instances. And there are
a lot more private than public instances.

Other arguments for this change are that `firejail` does not work inside
Docker and that documentation and setup become simpler.

To enable sandboxed execution, `--disable-sandbox=false` or the newly
added `--enable-sandbox` option can be used.
@fthomas fthomas added the enhancement New feature or request label Nov 6, 2020
@fthomas fthomas added this to the 0.8.0 milestone Nov 6, 2020
@codecov
Copy link

codecov bot commented Nov 6, 2020

Codecov Report

Merging #1719 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1719   +/-   ##
=======================================
  Coverage   73.99%   73.99%           
=======================================
  Files         115      115           
  Lines        1957     1957           
  Branches       55       50    -5     
=======================================
  Hits         1448     1448           
  Misses        509      509           
Impacted Files Coverage Δ
.../scala/org/scalasteward/core/application/Cli.scala 100.00% <ø> (ø)
...ala/org/scalasteward/core/application/Config.scala 89.65% <100.00%> (ø)
...in/scala/org/scalasteward/core/io/ProcessAlg.scala 100.00% <100.00%> (ø)

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 8e304de...d996533. Read the comment docs.

@fthomas fthomas merged commit 6b4b1dc into master Nov 6, 2020
@fthomas fthomas deleted the topic/change-default-of-disableSandbox branch November 7, 2020 08:56
@@ -118,7 +118,7 @@ object Config {
sandboxCfg = SandboxCfg(
whitelistedDirectories = args.whitelist,
readOnlyDirectories = args.readOnly,
disableSandbox = args.disableSandbox
enableSandbox = args.enableSandbox.getOrElse(args.disableSandbox)
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔

- enableSandbox = args.enableSandbox.getOrElse(args.disableSandbox)
+ enableSandbox = args.enableSandbox.getOrElse(!args.disableSandbox)

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦 Of course! Thanks for noticing! If you want to, you can submit a PR changing this. Or I'll do it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants