-
Notifications
You must be signed in to change notification settings - Fork 170
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
[sandbox] environment policies have the same env as policy loader #596
Conversation
So policies can detect if they are being executed on the CLI and decide to do something different like not loading themselves. Because some might require shared dict that is not available.
3489fbf
to
8433f54
Compare
arg
8433f54
to
228bf16
Compare
2855f21
to
de53506
Compare
@@ -0,0 +1,255 @@ | |||
--- Sandbox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactor 👍
I think this module does not contain any apicast-specific code so it could potentially be extracted into its own library, and that's why it's in the resty directory. However, there are some comments that mention apicast and policies. I think we should get rid of them. For example, the description of the module mentions APIcast, and the comment for local env
mentions policies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I did not want to bother much with it now, but rather when it is extracted. Would that be ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
tonumber = tonumber, tostring = tostring, os = { getenv = resty_env.value }, | ||
pcall = pcall, require = require, assert = assert, error = error, | ||
}) | ||
local box = sandbox.new() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a comment explaining why we need the sandbox here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. We don't need one. But is is quite convenient way of adding some extra env and not using global variables. Will add it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
* to split concerns of policy loading and a sandbox
de53506
to
df7011f
Compare
Extracts sandbox from policy loader to own module.
Load environment configurations with the same sandbox env as policies.
Expose
arg
in sandbox.Expose also
cli
as an alias toarg
when loading environment configurations.So policies can detect if they are being executed on the CLI
and decide to do something different like not loading themselves.
Because some might require shared dict that is not available.