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

refactor: rename --list-all-pkgs to --scanners pkg #5255

Closed
wants to merge 12 commits into from
Closed

refactor: rename --list-all-pkgs to --scanners pkg #5255

wants to merge 12 commits into from

Conversation

DmitriyLewen
Copy link
Contributor

@DmitriyLewen DmitriyLewen commented Sep 27, 2023 •
edited
Loading

Description

See #5183

TODO:

  • fix tests
  • update integration tests
  • update docs

Related issues

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).

@DmitriyLewen DmitriyLewen self-assigned this Sep 27, 2023
knqyf263
knqyf263 reviewed Sep 27, 2023
View reviewed changes
rpc/scanner/service.proto Outdated
Comment on lines 28 to 30
bool list_all_packages = 3;
map<string, Licenses> license_categories = 4;
bool include_dev_deps = 5;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The number should not be changed. We should use reserve.
https://protobuf.dev/programming-guides/proto3/#assigning

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 doubted this decision, but decided to think about it tomorrow. Thank you for saving me time!
Fixed in 6b6a6cc

pkg/types/target.go Outdated
@@ -43,6 +43,9 @@ const (

// LicenseScanner is the scanner of licenses
LicenseScanner = Scanner("license")

// SbomScanner includes all found packages on report
SbomScanner = Scanner("sbom")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
SbomScanner = Scanner("sbom")
SBOMScanner = Scanner("sbom")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 6b6a6cc

@DmitriyLewen DmitriyLewen marked this pull request as ready for review September 28, 2023 08:11
@DmitriyLewen DmitriyLewen marked this pull request as draft September 28, 2023 09:39
@DmitriyLewen DmitriyLewen marked this pull request as ready for review September 28, 2023 11:47
@knqyf263
Copy link
Collaborator

knqyf263 commented Oct 1, 2023

@DmitriyLewen Does this change make sense to you?

@DmitriyLewen
Copy link
Contributor Author

When i started working on this PR - I wasn't sure we needed these changes.
Previously I used --list-all-pkgs flag. I got used to it, it was convenient for me and this flag was enough for me.

But when i created this PR i understood your opinion about this change.

We have scanners for vuln, license, secrets, etc... => we also need scanners to find installed packages.

But i had 1 problem. Many times i understood that i was confusing sbom scanner and sbom target. This can really confuse users.
Perhaps we can rename --scanners sbom to --scanners packages or something like that. This may help with tangling.

@knqyf263
Copy link
Collaborator

knqyf263 commented Oct 2, 2023

We have scanners for vuln, license, secrets, etc... => we also need scanners to find installed packages.

Right. For example, there is a case where Trivy supports packages, but doesn't support vulnerabilities. It's easy for us to say --scanner sbom is supported, but --scanner vuln is not supported. It is more consistent.

Perhaps we can rename --scanners sbom to --scanners packages or something like that. This may help with tangling.

I like it. Let's go with that idea. Thanks!

@DmitriyLewen
Copy link
Contributor Author

I like it. Let's go with that idea. Thanks!

Okay, i will change sbom to packages and write to you.

@knqyf263
Copy link
Collaborator

knqyf263 commented Oct 2, 2023

Okay, i will change sbom to packages and write to you.

How about --scanner pkg?

@DmitriyLewen
Copy link
Contributor Author

DmitriyLewen commented Oct 2, 2023 •
edited
Loading

--scanners pkgs would be correct. ( we use license, secret, so we need to use pkg)
But I like. It's shorter than packages, but intuitive.

@DmitriyLewen DmitriyLewen changed the title refactor: rename --list-all-pkgs to --scanners sbom refactor: rename --list-all-pkgs to --scanners pkg Oct 4, 2023
@DmitriyLewen
Copy link
Contributor Author

@knqyf263 I renamed scanner. Take a look, when you have time, please

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 4, 2023

This PR is stale because it has been labeled with inactivity.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Dec 4, 2023
@DmitriyLewen DmitriyLewen removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Dec 4, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 3, 2024

This PR is stale because it has been labeled with inactivity.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Feb 3, 2024
@DmitriyLewen DmitriyLewen removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Feb 5, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 6, 2024

This PR is stale because it has been labeled with inactivity.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Apr 6, 2024
@DmitriyLewen DmitriyLewen removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Apr 15, 2024
@knqyf263
Copy link
Collaborator

knqyf263 commented Jun 4, 2024

We need to rethink the design.

@knqyf263 knqyf263 closed this Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knqyf263 knqyf263 knqyf263 left review comments

Assignees

@DmitriyLewen DmitriyLewen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename --list-all-pkgs to --scanners sbom
2 participants
@DmitriyLewen @knqyf263