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

Add function to read files from fs.FS to create bundle #3489

Closed
simongottschlag opened this issue May 25, 2021 · 4 comments · Fixed by #3493
Closed

Add function to read files from fs.FS to create bundle #3489

simongottschlag opened this issue May 25, 2021 · 4 comments · Fixed by #3493

Comments

@simongottschlag
Copy link
Contributor

Hi!

First of all, thank you for everything you do and all the great work with OPA. I'm using OPA in so many projects in many different forms and it is such a powerful tool.

I've been working on a way to dynamically create bundles using Go and it's been a pleasure working with your lib as well, it's one of the better ones I've had the pleasure of working with.

I am combining bundles from both embed and an API. When using NewDirectoryLoader(), I need to first create all the files in a temporary directory and then pass this directory to the function.

My proposal is to create an additional function, NewFsLoader(files fs.FS) that takes an FS interface as input:
https://golang.org/pkg/io/fs/#FS

This would make it possible for the end user of the function to handle how they want to expose files to your function. In my case, that would remove the necessity of creating a temp directory and instead just handle the logic dynamically with the interface.

What are your thoughts around this? If you would like to, I could give it a try and create a PR.

@srenatus
Copy link
Contributor

Hey there.

This sounds like a useful addition to me. One thing to consider is that we're trying to keep OPA buildable (as a Go module) from any supported Go version. As of today, that's 1.15 and 1.16 -- so we'd have to stub your addition out (probably using build tags) when the module is build with 1.15.

I think Go 1.17 is still a few months away, maybe August? Guessing from the release history... I suppose that when 1.15 is EOL'ed, we could refactor the existing logic backing DirectoryLoader to use the FS-based code path, too...? 🤔

@tsandall
Copy link
Member

@simongottschlag thanks for the kind words!

I agree w/ @srenatus that we want to maintain compatibility w/ older versions of Go (e.g., we don't want to break users that haven't updated yet.) Stubbing out the fs.FS dependency makes sense. Hopefully we can keep the amount of stubbed out code to a minimum.

@simongottschlag
Copy link
Contributor Author

Hi @tsandall and @srenatus!

I've created an initial PR: #3493 . I'm no expert when it comes to this, but I wanted to try and offload the majority of the work from you - if possible.

Feel free to give feedback on whatever you see, as I learn by doing and your expertise will of course be appriciated and warmly welcomed!

@srenatus
Copy link
Contributor

Thanks! I'll review this tomorrow at the latest.

simongottschlag added a commit to simongottschlag/opa that referenced this issue May 30, 2021
To make it possible to create a DirectoryLoader based on the fs.FS interface.
This interface requires go version 1.16 or above.

Fixes: open-policy-agent#3489
Signed-off-by: Simon Gottschlag <simon.gottschlag@xenit.se>
srenatus pushed a commit that referenced this issue May 31, 2021
To make it possible to create a DirectoryLoader based on the fs.FS interface.
This interface requires go version 1.16 or above.

Fixes: #3489

Signed-off-by: Simon Gottschlag <simon.gottschlag@xenit.se>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants