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

adding packages.props and Directory.packages.props parser #268

Merged
merged 28 commits into from
Nov 15, 2023

Conversation

yuriShafet
Copy link
Contributor

No description provided.

Copy link
Collaborator

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

Hello @yuriShafet
Thanks for work!

I left comments. Take a look, when you have time, please.

UPD:
1 more question - Can I use local packages? How does NuGet add them?

pkg/nuget/packages_props/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packages_props/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packages_props/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packages_props/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packages_props/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packages_props/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packages_props/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packages_props/parse_test.go Outdated Show resolved Hide resolved
pkg/nuget/packages_props/testdata/packages.props Outdated Show resolved Hide resolved
pkg/nuget/packages_props/parse.go Outdated Show resolved Hide resolved
@yuriShafet
Copy link
Contributor Author

@DmitriyLewen I pushed updated version. Please resolve conversations that you think is resolved done and add more comments if necessary.

Copy link
Collaborator

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

Hello @yuriShafet
Left comments.

Did you check local packages (#268 (review))?

pkg/nuget/packagesprops/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packagesprops/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packagesprops/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packagesprops/parse.go Show resolved Hide resolved
pkg/nuget/packages_props/parse.go Outdated Show resolved Hide resolved
return &Parser{}
}

func (p *Parser) addPackage(libs* []types.Library, pkg propsPackageEntry) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I didn't think about that when I wrote this comment:
What if we just use Library name?

func library(pkg entry) types.Library {

pkg/nuget/packagesprops/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packagesprops/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packagesprops/parse.go Outdated Show resolved Hide resolved
@yuriShafet
Copy link
Contributor Author

yuriShafet commented Nov 10, 2023

Hello @yuriShafet Left comments.

Did you check local packages (#268 (review))?

You mean packages that located in some local folder and nuget is configured to look into the folder or projects in same solution?
If you mean packages that located locally, it should work with that kind of packages.

Also, I pushed changes that should resolve your comments.

Copy link
Collaborator

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

Unfortunately, GitHub doesn’t allow me to make changes to your PR (maybe it is because you are using main branch)

I left a few changes
Check that they don't break anything and merge them please

pkg/nuget/packagesprops/parse.go Show resolved Hide resolved
pkg/nuget/packagesprops/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packagesprops/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packagesprops/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packagesprops/parse.go Outdated Show resolved Hide resolved
@yuriShafet
Copy link
Contributor Author

yuriShafet commented Nov 11, 2023

Unfortunately, GitHub doesn’t allow me to make changes to your PR (maybe it is because you are using main branch)

I left a few changes Check that they don't break anything and merge them please

I merged your changes and pushed the updated version. Thanks for the comments.

pkg/nuget/packagesprops/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packagesprops/parse.go Outdated Show resolved Hide resolved
pkg/nuget/packagesprops/parse_test.go Outdated Show resolved Hide resolved
@yuriShafet
Copy link
Contributor Author

@DmitriyLewen
Resolved the comments

Copy link
Collaborator

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

@yuriShafet Great! Thanks!

PR looks good to me now.

Thanks for your work and sorry that i had to ask you to make all small changes.
To add options for maintainers to update your PR - it is better to create new PRs in a different branch (not the main).

@knqyf263 I approved this PR. Can you take a look, when you have time.

@knqyf263
Copy link
Collaborator

@DmitriyLewen I trust you 👍

@knqyf263 knqyf263 merged commit 89e3bab into aquasecurity:main Nov 15, 2023
1 check passed
@DmitriyLewen
Copy link
Collaborator

@yuriShafet Can you create PR for Trivy now?
Some help info - https://aquasecurity.github.io/trivy/v0.47/community/contribute/pr/

@yuriShafet
Copy link
Contributor Author

@DmitriyLewen Will Do. Thanks

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.

3 participants