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

feat(#556): added check circular dependencies #772

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

renanbastos93
Copy link

Added validation to ensure that dependencies do not form circular dependencies.
An error is raised if such a condition is detected.

ISSUE: #556
Repository reference to simulate it: https://github.com/renanbastos93/weaver-issue-556

@renanbastos93 renanbastos93 changed the title feat(#556): added check circular dependecies feat(#556): added check circular dependencies Jun 7, 2024
@renanbastos93
Copy link
Author

@mwhittaker @sysulq @rgrandl

Does anyone have comments on this? Is it good, or do I need to change this commit?

@rgrandl
Copy link
Collaborator

rgrandl commented Jun 12, 2024

@renanbastos93, thanks for the PR.

Few comments:

  1. We already have a graph package, so we would prefer not to get a dependency on another graph library (github.com/dominikbraun/graph)
  2. A better place to check for circular dependencies is in the deployer. For example, in the multi deployer, you can do the check when you read the component graph https://github.com/ServiceWeaver/weaver/blob/main/internal/tool/multi/deployer.go#L250. Similar path should exist for the other deployers.

@renanbastos93
Copy link
Author

@renanbastos93, thanks for the PR.

Few comments:

  1. We already have a graph package, so we would prefer not to get a dependency on another graph library (github.com/dominikbraun/graph)
  2. A better place to check for circular dependencies is in the deployer. For example, in the multi deployer, you can do the check when you read the component graph https://github.com/ServiceWeaver/weaver/blob/main/internal/tool/multi/deployer.go#L250. Similar path should exist for the other deployers.

awesome, i will use that. Thanks a lot soon i will do a new commit.

Refactored the code to use the internal graph of the library
instead of relying on third-party libraries to validate cyclic
imports between components. This improves maintainability and
reduces unnecessary dependencies. Also, added unit tests to
ensure the validation works correctly.

- Uses the library's internal graph to validate cyclic imports
between components
- Removes dependency on third-party libraries for this functionality
- Adds unit tests to ensure the accuracy of the validation
- Adjusts the code according to the code review suggestions
@renanbastos93
Copy link
Author

@rgrandl done, check please.

Copy link
Collaborator

@rgrandl rgrandl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply on this, Renan.

In dfs.go, we have a dfs function that does a DFS traversal of the graph already. Is it possible to modify that function to detect cycles instead of implementing another traversal?

@renanbastos93
Copy link
Author

Sorry for the late reply on this, Renan.

In dfs.go, we have a dfs function that does a DFS traversal of the graph already. Is it possible to modify that function to detect cycles instead of implementing another traversal?

No worries! I can do this, but I will need more time because I've started a new job and my free time has been limited.

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 this pull request may close these issues.

2 participants