Add flag to follow symlinks when running opa build #6800
Conversation
tjons
force-pushed
the
tjons/6495-symlinks-building-bundles
branch
from
32341ef to
5de7d4b
Compare
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
tjons
force-pushed
the
tjons/6495-symlinks-building-bundles
branch
from
b58f28c to
6ee5e4c
Compare
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
tjons
force-pushed
the
tjons/6495-symlinks-building-bundles
branch
from
8329087 to
6bd3b39
Compare
loader/loader.go
Outdated
| } | ||
|
|
||
| // GetBundleDirectoryLoaderFS returns a bundle directory loader which can be used to load | ||
| // files in the directory. | ||
| func GetBundleDirectoryLoaderFS(fsys fs.FS, path string, filter Filter) (bundle.DirectoryLoader, bool, error) { | ||
| func GetBundleDirectoryLoaderFS(fsys fs.FS, path string, filter Filter, followSymlinks bool) (bundle.DirectoryLoader, bool, error) { |
There was a problem hiding this comment.
This will be a breaking change for anyone using this public function directly, although probably unlikely. Given that we also add WithFollowSymlinks(), couldn't callers simply use that on the returned value instead?
There was a problem hiding this comment.
great idea! I didn't think about the library usecase.
loader/loader.go
Outdated
| @@ -188,6 +190,12 @@ func (fl *fileLoader) WithRegoVersion(version ast.RegoVersion) FileLoader { | |||
| return fl | |||
| } | |||
|
|
|||
| // WithFollowSymLinks enables or disables following symlinks when loading files | |||
There was a problem hiding this comment.
| // WithFollowSymLinks enables or disables following symlinks when loading files | |
| // WithFollowSymlinks enables or disables following symlinks when loading files |
|
|
||
| params := newBuildParams() | ||
| params.outputFile = path.Join(rootDir, "test.tar.gz") | ||
| params.bundleMode = true |
There was a problem hiding this comment.
Can we drop This? Reading in bundle-mode doesn't seem like a requirement.
There was a problem hiding this comment.
Hm, I think I might have lead you astray with this coment 🤔 .
If I add params.bundleMode = true here, the test starts to fail.
But if I also add params.followSymlinks = true, it succeeds again, clearly showing that your code is being exercised.
So we need to add
params.bundleMode = true
params.followSymlinks = true
|
@johanfylling thanks for the review! sorry it took me a while to respond to your feedback... I think I have it all covered here! Let me know if anything else is needed before I squash all this :) |
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
tjons
force-pushed
the
tjons/6495-symlinks-building-bundles
branch
from
a84fabf to
568dc5d
Compare
There was a problem hiding this comment.
Thank you for the updates 🙂.
I think I misread the bundle-mode angle before.
Your tests look good, but it doesn't look like they're actually enabling params.followSymlinks = true, so they're showing this is a current feature for non-bundle-mode builds. If we add params.bundleMode = true to the tests, they start failing; and then adding params.followSymlinks = true, TestBuildWithFollowSymlinks will start succeeding again, showing that the your changes are necessary 👍 . The TestBuildWithFollowSymlinksEntireDir test will keep failing, however.
tjons
force-pushed
the
tjons/6495-symlinks-building-bundles
branch
from
568dc5d to
cb2de1f
Compare
…undles Signed-off-by: Tyler Schade <[email protected]>
tjons
force-pushed
the
tjons/6495-symlinks-building-bundles
branch
from
cb2de1f to
3496456
Compare
Why the changes in this PR are needed?
Per #6495, it is important to users using build systems like babel to allow
opa buildto include symlinked files in the bundle directory. Bazel and other build tools create sandboxes of symlinked files before building. As it stands today,opa buildonly includes regular files and directories by default.What are the changes in this PR?
This PR adds a boolean flag,
--follow-symlinksto theopa buildcommand to allow users to build directories with symlinked files, and have the contents of those symlinked files included in the built bundle.Notes to assist PR review:
I've included tests for the build command but would appreciate a critical eye as to any other areas that could use testing. Also, I've kept the symlink changes as constrained to the
buildfeatures as possible, but there are many layers of method calls to get to the core filesystem loading logic, so any ideas for cleaning up this code is welcome.Further comments: