-
Notifications
You must be signed in to change notification settings - Fork 32
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
Add file addon #20
base: master
Are you sure you want to change the base?
Add file addon #20
Conversation
if err := starlark.UnpackArgs(b.Name(), args, kwargs, unpacked...); err != nil { | ||
return nil, err | ||
} | ||
dat, err := ioutil.ReadFile(filename) |
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 guess this gets the work done, but filename
here is either absolute (which is not user-friendly since different people clone the same configuration repo at different path) or relative to the current working dir (which yields surprising results, since load()
function in starlark is relative to the current starlark file enclosing this particular load()
.)
Perhaps you could hack together something that ensures the relativity is interpreted consistently in the same starlark file by changing https://github.com/cruise-automation/isopod/blob/master/pkg/loader/loader.go.
Also, would be great to add some tests so we are use this implementation is correct.
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.
@cxuu do you mean to merge the behavior into the loader or to handle relative paths the same way that is done 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.
I was trying to say that load()
and file.read()
should "handle relative paths the same way". You could reuse if possible the code in loader.go
@@ -38,6 +38,7 @@ import ( | |||
// Plugin imports for auth. | |||
_ "k8s.io/client-go/plugin/pkg/client/auth/gcp" | |||
|
|||
fileaddon "github.com/cruise-automation/isopod/pkg/file" |
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.
nit: in starlark lingo this is called a "built-in"
This adds an addon called
file
that allows you to read an arbitrary file. This is certainly not hermetic, but i figure that hermeticity went out the window a long time ago since we do API requests and whatnot.Example:
It's sort of a bummer that this doesn't happen during the loading phase, but I don't know how to change that.