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: set InstalledFiles for DEB and RPM packages #5488

Merged
merged 5 commits into from
Nov 16, 2023

Conversation

lebauce
Copy link
Contributor

@lebauce lebauce commented Nov 1, 2023

Description

This PR does 2 main things :

  • Currently, only apk package analyzer attaches the installed files to the Package structure. This PR does the same for DEB and RPM.
  • It puts this installed file collection behind an artifact option as not everyone may be interested in collecting these files.

Related issues

  • Close #XXX

Related PRs

  • #XXX
  • #YYY

Remove this section if you don't have related PRs.

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@knqyf263 knqyf263 marked this pull request as draft November 2, 2023 01:51
@knqyf263
Copy link
Collaborator

knqyf263 commented Nov 2, 2023

@lebauce Thanks for your contribution! It looks like WIP. I converted it to a draft. Please let us know when it is ready for review.

@lebauce
Copy link
Contributor Author

lebauce commented Nov 2, 2023

@knqyf263 No problem. The feature itself should work but I'm struggling with updating the fixtures of integration tests. Working on it

@lebauce lebauce force-pushed the package-install-files branch 7 times, most recently from 68f01b5 to fffe339 Compare November 6, 2023 12:58
@lebauce lebauce marked this pull request as ready for review November 6, 2023 13:40
@lebauce
Copy link
Contributor Author

lebauce commented Nov 6, 2023

@knqyf263 I fixed the tests and undrafted the PR

@@ -146,6 +146,7 @@ type PostAnalysisInput struct {
type AnalysisOptions struct {
Offline bool
FileChecksum bool
CollectFiles bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can collect the files by default. I mean we may not need the option. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍
I updated the PR without the option

@lebauce lebauce force-pushed the package-install-files branch 2 times, most recently from 5e3eb4a to 38eef9d Compare November 9, 2023 08:35
@lebauce lebauce requested a review from knqyf263 November 9, 2023 10:30
@knqyf263
Copy link
Collaborator

Thanks for updating the tests, but a massive list of expected values is not our intention. I created a PR to improve the RPM tests.
#5567

I'll create a PR improving VM tests as well. After that, we can add better tests to this PR.

Signed-off-by: knqyf263 <knqyf263@gmail.com>
@knqyf263 knqyf263 added this pull request to the merge queue Nov 16, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 16, 2023
@knqyf263 knqyf263 added this pull request to the merge queue Nov 16, 2023
Merged via the queue into aquasecurity:main with commit 44d0b28 Nov 16, 2023
12 checks passed
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.

None yet

2 participants