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: return depgraph from plugin #159

Merged
merged 1 commit into from
Aug 24, 2021
Merged

feat: return depgraph from plugin #159

merged 1 commit into from
Aug 24, 2021

Conversation

admons
Copy link
Contributor

@admons admons commented Aug 12, 2021

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

this change will return a depgraph instead of a deptree from the plugin

@admons admons requested review from a team and orsagie and removed request for a team August 12, 2021 12:09
@admons admons requested a review from a team as a code owner August 12, 2021 12:09
@CLAassistant
Copy link

CLAassistant commented Aug 12, 2021

CLA assistant check
All committers have signed the CLA.

@admons admons force-pushed the fix/return_depgraph branch 3 times, most recently from 96cf604 to dd01624 Compare August 12, 2021 15:54
Copy link
Contributor

@jan-stehlik jan-stehlik left a comment

Choose a reason for hiding this comment

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

nice one @admons 🎉

Comment on lines +837 to +843
.then(async (result) => {
const dependencyGraph = result.dependencyGraph;
const pkg = await depGraphLib.legacy.graphToDepTree(
dependencyGraph,
'pip'
);
t.ok(pkg.dependencies, 'does not error');
Copy link
Contributor

Choose a reason for hiding this comment

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

these are a bit cheeky assertions haha, but lgtm

@admons admons force-pushed the fix/return_depgraph branch 5 times, most recently from 28b21d7 to 35f3749 Compare August 24, 2021 04:02
this change will return a depgraph instead of a deptree from the plugin

because a package can only have one version, it doesn't make sense to create it more than once in "pip_resolve.py" script
this change will remove the duplicates from the scripts and replace them with a "true" value
later the "buildDepGraph" function will iterate over the tree and replace all "true" values with a reference to the original package

this will improve performance for those reasons:
1. no serializing dep tree with repeating dependencies
2. no deserializing repeating dependencies (memory and cpu affects)
3. return a depGraph
@admons admons force-pushed the fix/return_depgraph branch from 35f3749 to d58d135 Compare August 24, 2021 04:04
@admons admons merged commit 1d93720 into master Aug 24, 2021
@admons admons deleted the fix/return_depgraph branch August 24, 2021 08:31
@snyksec
Copy link

snyksec commented Aug 24, 2021

🎉 This PR is included in version 1.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

admons added a commit that referenced this pull request Sep 9, 2021
In #159 we introduced a change to use dep graph instead of dep tree
In specific scenarios, we see a direct dependency more than once for a specific package.
This change will make sure we're setting it correctly
admons added a commit that referenced this pull request Sep 9, 2021
In #159 we introduced a change to use dep graph instead of dep tree
In specific scenarios, we see a direct dependency more than once for a specific package.
This change will make sure we're setting it correctly
admons added a commit that referenced this pull request Sep 9, 2021
In #159 we introduced a change to use dep graph instead of dep tree
In specific scenarios, we see a direct dependency more than once for a specific package.
This change will make sure we're setting it correctly
admons added a commit that referenced this pull request Sep 9, 2021
In #159 we introduced a change to use dep graph instead of dep tree
In specific scenarios, we see a direct dependency more than once for a specific package.
This change will make sure we're setting it correctly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants