-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix follow path checking at depths greater than 2 #8819
Conversation
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 cherry-picked the test onto current master and it fails indeed with error: input 'B/D' follows a non-existent input 'B/C/D'
. I then tested the full branch locally and it does indeed fix the test.
Code change itself is trivial, this can be merged.
If you want, you can also add a bullet to rl-next.md
so the fix shows up in the next release notes, but it's not a requirement :)
Sure - I've added a small note. |
We need to recurse into the input tree to handle follows paths that trarverse multiple inputs that may or may not be follow paths themselves.
7396f74
to
37a509c
Compare
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.
Looks good! Just to set your expectations, I don't have commit access, so this PR might stay open until one of the maintainers gets around to merging it, despite my approval.
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.
Tricky one. I think the diagram I wrote in the suggestion could be helpful. Does it match your understanding of the problem?
@tomberek I think you mentioned a problem like this some time ago.
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
The example I've been using: #5790 (comment) |
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.
Thanks for making this PR @VertexA115 <3
Co-authored-by: Alex Ameen <alex.ameen.tx@gmail.com>
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.
It would perhaps be nice to (also?) have a test that's based on the actual contents of the overridden flakes; maybe something from #5790?
Nonetheless the new test seems sufficient as a regression test.
The test on macos is failing only due to a flaky test. Could someone with CI access retrigger it? |
Note. This PR is solving a different problem than what I mentioned above. |
Motivation
Currently, you can't have follows paths with depths greater than 2 - you get an error along the lines of
error: input 'B/D' follows a non-existent input 'B/C/D'
.Context
We need to recurse into the input tree to handle follows paths that trarverse multiple inputs that may or may not be follow paths themselves. I've added tests for the problem.
Checklist for maintainers
Maintainers: tick if completed or explain if not relevant
tests/**.sh
src/*/tests
tests/nixos/*
Priorities
Add 👍 to pull requests you find important.