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

Get Next Actionable Critical Dependencies Part 1 #1705

Merged
merged 9 commits into from
May 23, 2024

Conversation

nathannaveen
Copy link
Contributor

@nathannaveen nathannaveen commented Feb 12, 2024

Description of the PR

PR Checklist

  • All commits have a Developer Certificate of Origin (DCO) -- they are generated using -s flag to git commit.
  • All new changes are covered by tests
  • If GraphQL schema is changed, make generate has been run
  • If collectsub protobuf has been changed, make proto has been run
  • All CI checks are passing (tests and formatting)
  • All dependent PRs have already been merged

@nathannaveen
Copy link
Contributor Author

@mdeicas 👀

@nathannaveen nathannaveen force-pushed the nathan/nextActionableCriticalDep2 branch from b70ef5f to 7032600 Compare February 12, 2024 21:00
@nathannaveen nathannaveen force-pushed the nathan/nextActionableCriticalDep2 branch 4 times, most recently from fd88f21 to a296ed7 Compare February 21, 2024 17:38
Copy link
Collaborator

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

Looks good, one nit

pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
@lumjjb
Copy link
Contributor

lumjjb commented Feb 25, 2024

@mdeicas , would you be able to help review this?

pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents_test.go Show resolved Hide resolved
pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents.go Show resolved Hide resolved
Copy link
Collaborator

@mdeicas mdeicas left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @nathannaveen!! I added some comments on the code but also wanted to ask about the granularity of the dependency relationship at a higher level (see the comment on this).

pkg/guacrest/server/server.go Outdated Show resolved Hide resolved
pkg/guacrest/server/server.go Outdated Show resolved Hide resolved
pkg/guacrest/server/server.go Outdated Show resolved Hide resolved
pkg/guacrest/server/server.go Outdated Show resolved Hide resolved
pkg/guacrest/openapi.yaml Outdated Show resolved Hide resolved
pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents.go Show resolved Hide resolved
pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents_test.go Show resolved Hide resolved
pkg/dependencies/dependents_test.go Show resolved Hide resolved
@nathannaveen nathannaveen force-pushed the nathan/nextActionableCriticalDep2 branch 5 times, most recently from 280628a to fb8911d Compare March 7, 2024 16:06
@nathannaveen nathannaveen force-pushed the nathan/nextActionableCriticalDep2 branch 2 times, most recently from c47967d to af96d18 Compare April 2, 2024 15:09
@nathannaveen nathannaveen force-pushed the nathan/nextActionableCriticalDep2 branch 2 times, most recently from ac00d9a to 3211c93 Compare April 3, 2024 13:58
@pxp928
Copy link
Collaborator

pxp928 commented Apr 3, 2024

@nathannaveen once PR #1807 merged, rebase and your tests should pass.

@nathannaveen nathannaveen force-pushed the nathan/nextActionableCriticalDep2 branch from 3211c93 to 13ca53e Compare April 5, 2024 14:39
@nathannaveen
Copy link
Contributor Author

@nathannaveen once PR #1807 merged, rebase and your tests should pass.

Thanks @pxp928!

pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents.go Show resolved Hide resolved
pkg/dependencies/dependents.go Show resolved Hide resolved
cmd/guacone/cmd/vulnerability.go Outdated Show resolved Hide resolved
@nathannaveen nathannaveen force-pushed the nathan/nextActionableCriticalDep2 branch from 13ca53e to cf32140 Compare April 19, 2024 21:35
Copy link
Collaborator

@mdeicas mdeicas left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes @nathannaveen! A few more minor comments.

pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
pkg/guacrest/server/server.go Show resolved Hide resolved
@mdeicas mdeicas mentioned this pull request Apr 24, 2024
4 tasks
@nathannaveen nathannaveen force-pushed the nathan/nextActionableCriticalDep2 branch from cf32140 to 1499be4 Compare May 10, 2024 23:24
@nathannaveen
Copy link
Contributor Author

Everyone, thanks for your reviews!

This PR has been open for a pretty long time, so if there are any nits in the future, can we create an issue for them so that we can get this PR merged in?

@pxp928 pxp928 requested a review from mdeicas May 13, 2024 13:37
Copy link
Collaborator

@mdeicas mdeicas left a comment

Choose a reason for hiding this comment

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

Aside from these two comments, LGTM @nathannaveen!!

pkg/dependencies/dependents_test.go Outdated Show resolved Hide resolved
pkg/dependencies/dependents.go Outdated Show resolved Hide resolved
@nathannaveen nathannaveen force-pushed the nathan/nextActionableCriticalDep2 branch from 1499be4 to c384f15 Compare May 16, 2024 17:59
* Got all dependencies using `hasSBOM`
* This finds the number of package that depends on each individual package
* Packages with different versions are still the same package

Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
@nathannaveen nathannaveen force-pushed the nathan/nextActionableCriticalDep2 branch from c384f15 to 539b7d6 Compare May 16, 2024 18:03
@kodiakhq kodiakhq bot merged commit 14cd291 into guacsec:main May 23, 2024
8 checks passed
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.

6 participants