Support nested manifests and relative paths #216
Conversation
roekatz
force-pushed
the
rk/enhance_manifests
branch
from
9cd165d
to
bb754bd
Compare
roekatz
force-pushed
the
rk/enhance_manifests
branch
2 times, most recently
from
069ac19
to
a2f6ee7
Compare
| # Path is relative to current directory, make it absolute | ||
| path_entry = dir.path / path_entry | ||
| if not viewer.exists(path_entry): | ||
| logger.warning(f"Path '{path_entry}' does not exist") |
There was a problem hiding this comment.
These log entries will become hard to understand/debug if we're dealing with a large, complex set of deeply-nested manifests. It would be better if it included the full path of how we got here, from the root manifest.
| """ | ||
| filterable_gen = filterable_gen or self.nodes | ||
| path = Path(path) # Works even if path is already Path | ||
| nodes = filterable_gen(lambda n: n.path == path) |
There was a problem hiding this comment.
I feel like itertools.filterfalse will help you here -- maybe it can replace this whole method.
There was a problem hiding this comment.
Not sure how filterfalse would help me, but filter actually helps.
My code there was unnecessarily complex - I've fixed it and I believe it's much more readable now (with a small price of having a slightly duplicated [line of] code - but I think it's justifiable)
| repo: Repo = local_repo | ||
| root = Path(repo.working_tree_dir) | ||
|
|
||
| # Create multiple recursive .manifest files |
There was a problem hiding this comment.
This test is great, but we also need some tests for failure cases. What happens when a manifest points to another manifest that doesn't exist? What if the manifest is binary instead of utf-8 text? What if the manifest is a symbolic link? What if we have a manifest cycle?
| repo: Repo = local_repo | ||
| root = Path(repo.working_tree_dir) | ||
|
|
||
| # Create multiple recursive .manifest files |
There was a problem hiding this comment.
This test is great, but we also need some tests for failure cases. What happens when a manifest points to another manifest that doesn't exist? What if the manifest is binary instead of utf-8 text? What if the manifest is a symbolic link? What if we have a manifest cycle?
There was a problem hiding this comment.
- Points to another manifest that doesn't exist - That's actually tested now (pointing to 'some' dir which is empty)
- Manifest cycle - That's not really possible because paths are always interpreted relatively to
.manifest's dir. But I'm adding a test for that with multiple hacking attempts (including ".."). - Binary/utf-8 - I've just added a test for testing my own changes, if that's an issue than it's not new. I'll see if it's easy to add one.
- Symbolic link - Same point as 3.
There was a problem hiding this comment.
More about symlinks:
I would say symlinks are probably fine if they point inside the repo.
If they point outside it is problematic.
We probably want to ignore symlinks altogether, just to be sure.
Also pay attention that the .manifest only generates the list by which the bundle files are sorted,
the main danger are data.json / *.rego symlinks pointing outside the repo (which can be used to fetch arbitrary files from server).
I'll check if that's an issue - and open a ticket for it if so (since that's outside the scope of that PR anyway)
There was a problem hiding this comment.
- in second thought the file always holds binary data. When using textual mode in python ("r" rather than "rb") it uses utf-8 by default to decode them.
If someone would use a different encoding for his manifest file - it would be equivalent of having non existing paths within the manifest.
But I feel like expecting "utf-8" is totally expected... What exactly do you think the behavior should be?
There was a problem hiding this comment.
- I haven't been able to use our current
Repoobject in order to commit symlinks (gave it 2 minutes). I've opened a ticket in linear for making sure we don't handle symlinks. Other than that I feel like that's a bit out of scope for merging this PR :)
roekatz
force-pushed
the
rk/enhance_manifests
branch
5 times, most recently
from
fe24faf
to
4f9da4d
Compare
roekatz
force-pushed
the
rk/enhance_manifests
branch
from
4f9da4d
to
b5e95aa
Compare
Nested menifests: minor CR fix Co-authored-by: David Baumgold <db@permit.io>
roekatz
force-pushed
the
rk/enhance_manifests
branch
from
ab6156e
to
422d45c
Compare
No description provided.