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.

Sign up for GitHub

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

Jump to bottom

Populate SourceInformation.DocumentRef in collectors #1847

Merged
merged 16 commits into from Apr 23, 2024
Merged

Populate SourceInformation.DocumentRef in collectors #1847

merged 16 commits into from Apr 23, 2024

Conversation

nchelluri
Copy link
Contributor

@nchelluri nchelluri commented Apr 16, 2024 •
edited

Description of the PR

Fixes #1846

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 OpenAPI spec 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

@nchelluri nchelluri force-pushed the nchelluri/add-set-blob-key-flag-to-all-collectors branch from 9c495b6 to 4f71a4a Compare April 16, 2024 19:12
pxp928
pxp928 approved these changes Apr 17, 2024
View reviewed changes
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.

Overall looks good! Thanks

pkg/handler/collector/deps_dev/deps_dev_test.go Outdated Show resolved Hide resolved
pkg/handler/collector/github/github_test.go Show resolved Hide resolved
pkg/handler/collector/s3/s3.go Outdated Show resolved Hide resolved
pkg/handler/collector/s3/s3.go Outdated Show resolved Hide resolved
pkg/handler/collector/s3/s3.go Outdated Show resolved Hide resolved
@nchelluri
Add store-blob-url CLI flag to all collectors
ca5514b 
Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Import package only once in file
4cd7aa9 
- This clears up a go-staticcheck warning

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Make use of setBlobURL flag in all collectors
b77d5eb 
Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Remove impossible conditional clause
3ddd526 
- This clears up a nilness analyzer warning

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Import package only once in file
85ae7ec 
- This clears up a go-staticcheck warning

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Tweak error message to not end with punctuation
227830a 
- This clears up up a go-staticcheck warning

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Add test for collector storeBlobURL flags
6b0438c 
- TODO: GitHub

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Fix typo in filename
5b6e355 
Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Add some TODOs to the GitHub collector test.
5d740cc 
Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Rename flag more appropriately
d0dcbca 
Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Always set DocumentRef to blob key
9cf5e3c 
- Since we now have the DocumentRef field, there is
  no reason to gate its usage, and this simplifies
  the CLI command code.

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Fix broken tests (minor)
d7ae35a 
- I forgot to include the DocumentRef values in my
  want docs.

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Minor refactor
deab375 
- Since we no longer branch based on whether or not we
  want to store blob keys (we always do it), the getDocRef()
  methods have become funcs instead.

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Fix S3 collected doc Source field
2e54659 
Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri
Fix SourceInformation assertion
1dc9ced 
Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri nchelluri force-pushed the nchelluri/add-set-blob-key-flag-to-all-collectors branch from e114cf3 to 1dc9ced Compare April 22, 2024 13:51
@nchelluri
Create helper wrapper func to aid readability
2c68586 
- This allows me to stop repeating a call with a comment
  explaining it multiple time throughout the collector
  code. It's  very minor change.

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@pxp928 pxp928 added the needs-review Needs writer LGTM label Apr 22, 2024
@kodiakhq kodiakhq bot merged commit 3577d4d into guacsec:main Apr 23, 2024
8 checks passed
@nchelluri nchelluri deleted the nchelluri/add-set-blob-key-flag-to-all-collectors branch April 23, 2024 15:22
arorasoham9 pushed a commit to arorasoham9/guac that referenced this pull request May 17, 2024
@nchelluri @arorasoham9
Populate SourceInformation.DocumentRef in collectors (guacsec#1847)
044b317 
* Add store-blob-url CLI flag to all collectors

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Import package only once in file

- This clears up a go-staticcheck warning

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Make use of setBlobURL flag in all collectors

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Remove impossible conditional clause

- This clears up a nilness analyzer warning

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Import package only once in file

- This clears up a go-staticcheck warning

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Tweak error message to not end with punctuation

- This clears up up a go-staticcheck warning

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Add test for collector storeBlobURL flags

- TODO: GitHub

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Fix typo in filename

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Add some TODOs to the GitHub collector test.

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Rename flag more appropriately

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Always set DocumentRef to blob key

- Since we now have the DocumentRef field, there is
  no reason to gate its usage, and this simplifies
  the CLI command code.

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Fix broken tests (minor)

- I forgot to include the DocumentRef values in my
  want docs.

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Minor refactor

- Since we no longer branch based on whether or not we
  want to store blob keys (we always do it), the getDocRef()
  methods have become funcs instead.

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Fix S3 collected doc Source field

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Fix SourceInformation assertion

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

* Create helper wrapper func to aid readability

- This allows me to stop repeating a call with a comment
  explaining it multiple time throughout the collector
  code. It's  very minor change.

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>

---------

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
Signed-off-by: Soham Arora <arorasoham9@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lumjjb lumjjb lumjjb approved these changes

@pxp928 pxp928 pxp928 approved these changes

@jeffmendoza jeffmendoza Awaiting requested review from jeffmendoza jeffmendoza is a code owner

Assignees
No one assigned
Labels
needs-review Needs writer LGTM size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[feature] The documentRef GraphQL field is populated by the collectors
3 participants
@nchelluri @lumjjb @pxp928