-
Notifications
You must be signed in to change notification settings - Fork 2k
Add self-ref check when build task graph #1141
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
Conversation
Someone is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
e39ca16
to
81ac2dd
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.
This is the right check, but there are two graphs: the package-dependency graph, and the package-task graph. You're adding self-referential checks to the package-task graph, and we have cycle detection on the package-dependency graph.
How would you feel about making a utility function that does cycle + self-referential checks and applying it to both? I've linked to the other one in a comment, and I think the utility function would be the combination of the two checks (almost exactly dag.Validate
, except the root check)
cli/internal/run/run.go
Outdated
@@ -376,6 +376,12 @@ func buildTaskGraph(topoGraph *dag.AcyclicGraph, pipeline fs.Pipeline, rs *runSp | |||
}); err != nil { | |||
return nil, err | |||
} | |||
|
|||
for _, e := range engine.TaskGraph.Edges() { |
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.
Can you add the cycle validation here as well? You can find an example here
418cbe5
to
c6e66ce
Compare
c6e66ce
to
deac761
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.
Awesome! Can you fix merge conflicts/lint and we'll get this merged?
Looks like |
@Kingwl it looks like there is one minor lint issue to be taken care of and then we can merge this. I'm also happy to take over the PR if need be. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Fixed #1117