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

Add gradle lockfile support #46

Merged
merged 1 commit into from
Dec 15, 2022
Merged

Add gradle lockfile support #46

merged 1 commit into from
Dec 15, 2022

Conversation

abhisek
Copy link
Contributor

@abhisek abhisek commented Dec 14, 2022

Add support for:

  1. gradle.lockfile
  2. buildscript-gradle.lockfile

Reference: https://docs.gradle.org/current/userguide/dependency_locking.html#lock_state_location_and_format

@google-cla
Copy link

google-cla bot commented Dec 14, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@another-rex
Copy link
Collaborator

/gcbrun

@another-rex
Copy link
Collaborator

Thank you for the PR!

@another-rex
Copy link
Collaborator

CC @G-Rath

G-Rath
G-Rath suggested changes Dec 15, 2022
View reviewed changes
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Awesome stuff! You need to update the list of lockfiles in the readme, and it would also be good to have a few tests covering a lockfile with a bad line (or two).

pkg/lockfile/parse-gradle-lock_test.go Outdated
Comment on lines 70 to 112
if len(packages) != 5 {
t.Errorf("Expected %d packages got %d", 5, len(packages))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use expectPackages here? I think it's worth asserting the actual packages to ensure they're all being parsed correctly

@abhisek abhisek force-pushed the feat/gradle-lockfile-support branch from d8a5231 to 5afef7e Compare December 15, 2022 05:13
@abhisek
Copy link
Contributor Author

abhisek commented Dec 15, 2022

@G-Rath Thanks for the suggestions. I have updated the PR.

@abhisek abhisek force-pushed the feat/gradle-lockfile-support branch from 5afef7e to 1daf873 Compare December 15, 2022 05:17
@abhisek abhisek requested a review from G-Rath December 15, 2022 07:13
G-Rath
G-Rath reviewed Dec 15, 2022
View reviewed changes
pkg/lockfile/parse-gradle-lock_test.go Outdated
func TestParseGradleLock_OnlyComments(t *testing.T) {
t.Parallel()

packages, err := lockfile.ParseGradleLock("fixtures/gradle/only-comment")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: technically this file should be only-comments, which aligns with the test name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed :)

G-Rath
G-Rath suggested changes Dec 15, 2022
View reviewed changes
Copy link
Collaborator

@G-Rath G-Rath 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 - just need to update the list in TestParse_FindsExpectedParsers

pkg/lockfile/parse_test.go
@@ -95,6 +96,7 @@ func TestParse_FindsExpectedParsers(t *testing.T) {
"poetry.lock",
"pubspec.lock",
"requirements.txt",
"gradle.lockfile",
Copy link
Collaborator

Choose a reason for hiding this comment

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

buildscript-gradle.lockfile should be included on this list (effectively this should have all lockfiles that are listed in the readme as being supported)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to include buildscript-gradle.lockfile. But had to decrement the asserted count because both gradle.lockfile and buildscript-gradle.lockfile use the same parser.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sweet as - that test is more meant to help catch people forgetting to update this section of tests if a new parser is added, which is always a bit fuzzy (arguably I should have used a map against the function name and then check that each was called at least once, which I think could be possible, but not worth the effort imo)

@abhisek
Add gradle lockfile support
ccdc7a0 
Fix test case

Add gradle lockfile support

Fix test case

Update pkg/lockfile/ecosystems_test.go

Co-authored-by: Gareth Jones <[email protected]>

Update test case as per PR review comment

Signed-off-by: abhisek <[email protected]>

Update README
@abhisek abhisek force-pushed the feat/gradle-lockfile-support branch from 1daf873 to ccdc7a0 Compare December 15, 2022 18:37
@abhisek abhisek requested a review from G-Rath December 15, 2022 18:38
@another-rex
Copy link
Collaborator

/gcbrun

@another-rex another-rex merged commit c16b770 into google:main Dec 15, 2022
@G-Rath G-Rath mentioned this pull request Dec 22, 2022
Merged
julieqiu pushed a commit to julieqiu/osv-scanner that referenced this pull request May 2, 2023
@abhisek @julieqiu
Add gradle lockfile support (google#46)
3e183c3 
Fix test case

Add gradle lockfile support

Fix test case

Update pkg/lockfile/ecosystems_test.go

Co-authored-by: Gareth Jones <[email protected]>

Update test case as per PR review comment

Signed-off-by: abhisek <[email protected]>

Update README
julieqiu pushed a commit to julieqiu/osv-scanner that referenced this pull request May 2, 2023
@abhisek @julieqiu
Add gradle lockfile support (google#46)
e3deeb5 
Fix test case

Add gradle lockfile support

Fix test case

Update pkg/lockfile/ecosystems_test.go

Co-authored-by: Gareth Jones <[email protected]>

Update test case as per PR review comment

Signed-off-by: abhisek <[email protected]>

Update README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@G-Rath G-Rath G-Rath approved these changes

@another-rex another-rex another-rex approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abhisek @another-rex @G-Rath