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(cli): rename --vuln-type flag to --pkg-types flag #7104

Merged
merged 6 commits into from
Jul 9, 2024

Conversation

DmitriyLewen
Copy link
Contributor

Description

Rename --vuln-type flag to --pkg-types flag.
See #6269

Before:

➜ trivy -d image --vuln-type os aquasec/trivy
...
2024-07-05T15:45:36+06:00       DEBUG   Vulnerability type      type=[os]
...
aquasec/trivy (alpine 3.20.0)

Total: 10 (UNKNOWN: 0, LOW: 0, MEDIUM: 10, HIGH: 0, CRITICAL: 0)

➜ trivy -d image --vuln-type library aquasec/trivy
...
2024-07-05T15:45:46+06:00       DEBUG   Vulnerability type      type=[library]
...

usr/local/bin/trivy (gobinary)

Total: 1 (UNKNOWN: 0, LOW: 0, MEDIUM: 1, HIGH: 0, CRITICAL: 0)

After:

➜ trivy -d image --vuln-type os aquasec/trivy
2024/07/05 15:47:26 WARN '--vuln-type' is deprecated. Use '--pkg-types' instead.

➜ trivy -d image --pkg-types os aquasec/trivy
...
2024-07-05T15:47:59+06:00       DEBUG   Package types   types=[os]
...

aquasec/trivy (alpine 3.20.0)

Total: 10 (UNKNOWN: 0, LOW: 0, MEDIUM: 10, HIGH: 0, CRITICAL: 0)

➜ trivy -d image --pkg-types library aquasec/trivy
...
2024-07-05T15:48:11+06:00       DEBUG   Package types   types=[library]
...

usr/local/bin/trivy (gobinary)

Total: 1 (UNKNOWN: 0, LOW: 0, MEDIUM: 1, HIGH: 0, CRITICAL: 0)

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 Jul 5, 2024
@DmitriyLewen DmitriyLewen changed the title refactor(cli)!: rename vuln-type to pkg-types refactor(cli)!: rename --vuln-type flag to --pkg-types flag Jul 5, 2024
@DmitriyLewen DmitriyLewen marked this pull request as ready for review July 5, 2024 10:21
@knqyf263
Copy link
Collaborator

knqyf263 commented Jul 5, 2024

We can leverage aliases.

// Aliases represents aliases
Aliases []Alias

Example:

Aliases: []Alias{
{
Name: "skip-policy-update",
Deprecated: true,
},
},

@DmitriyLewen
Copy link
Contributor Author

I used Alias.
But I thought the report.go file would be better suited for this flag.
So i moved this flag to this file + rename flag + add Alias.

PkgTypesFlag = Flag[[]string]{
Name: "pkg-types",
ConfigName: "pkg-types",
Default: types.PkgTypes,
Values: types.PkgTypes,
Usage: "comma-separated list of package types",
Aliases: []Alias{
{
Name: "vuln-type",
ConfigName: "vulnerability.type",
Deprecated: true, // --vuln-type was renamed to --pkg-types
},
},
}

@knqyf263
Copy link
Collaborator

knqyf263 commented Jul 9, 2024

I don't think it's a breaking
change. People can keep using --vuln-type. We should announce the deprecation. After a while, we'll delete it, then it'll be a breaking change as the same command no longer works.

It is always difficult to determine the type of PR, but I think it will be a "refactor" when there is no user impact. In this case, UI will be changed. It's not a new feature or bug fix, but I think "feat" is the closest.

@DmitriyLewen
Copy link
Contributor Author

DmitriyLewen commented Jul 9, 2024

I don't think it's a breaking change

I thought about this.
I renamed VulnType to PkgTypes e.g.:

These are breaking changes for users using Trivy as a library.

It is always difficult to determine the type of PR, but I think it will be a "refactor" when there is no user impact. In this case, UI will be changed. It's not a new feature or bug fix, but I think "feat" is the closest.

hm... okay, i will change type of PR.

@DmitriyLewen DmitriyLewen changed the title refactor(cli)!: rename --vuln-type flag to --pkg-types flag feat(cli)!: rename --vuln-type flag to --pkg-types flag Jul 9, 2024
@knqyf263
Copy link
Collaborator

knqyf263 commented Jul 9, 2024

These are breaking changes for users using Trivy as a library.

That's a good point. But Trivy is a CLI tool. We could consider whether changing to CLI usage is destructive. Of course, we should ensure the change is minimal for tools importing Trivy, though. Otherwise, adding arguments to an internal function will be a breaking change if it is exported.

@naortalmor1 @tamirkiviti13 @tonaim We changed a variable name. I don't think it will have a major impact, but I thought I'd let you know just in case.

@DmitriyLewen DmitriyLewen changed the title feat(cli)!: rename --vuln-type flag to --pkg-types flag feat(cli): rename --vuln-type flag to --pkg-types flag Jul 9, 2024
@DmitriyLewen
Copy link
Contributor Author

Okay. I got you.
I updated type of PR

Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

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

It's not an issue from this PR, but logging doesn't seem to be enabled well. I mean our custom handler is not initialized, and the log format is different.

2024/07/09 10:08:31 WARN '--vuln-type' is deprecated. Use '--pkg-types' instead.
2024-07-09T10:08:31+04:00	INFO	Vulnerability scanning is enabled

repeated string scanners = 2;
map<string, Licenses> license_categories = 4;
bool include_dev_deps = 5;
repeated string pkg_types = 6;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't have to add a new field when renaming it. Protobuf uses a number for marshaling/unmarshaling. When you change the field type, like int to string, you need a new field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! Updated in aeb4f49

@DmitriyLewen
Copy link
Contributor Author

It's not an issue from this PR, but logging doesn't seem to be enabled well. I mean our custom handler is not initialized, and the log format is different.

Created #7124 for this.

// VulnTypeLibrary is a vulnerability type of programming language dependencies
VulnTypeLibrary = VulnType("library")
// PkgTypeLibrary is a package type of programming language dependencies
PkgTypeLibrary = PkgType("library")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I want to change it to lang or language as we lately call them "Language-specific packages". (we used to call them libraries, as you know). But I don't want to break compatibility. Do you think we should open another PR? I think it's a good opportunity.

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 think it's a good opportunity.

Yeah. Now is the best time for this.

I want to change it to lang or language as we lately call them "Language-specific packages"

I think lang will be enough.

But I don't want to break compatibility. Do you think we should open another PR?

I think it is better to create one more PR for this.
Also we need to create 2 paragraphs in release discussion to attract the attention of users to the fact that we not only renamed flag, but also changed library to lang.
Also in the new PR we need to add warning when user uses library and rename it (like we do with flag aliases)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is better to create one more PR for this.

OK. However, we can announce these changes together in a single thread in GitHub Discussions.
e.g. #7010

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's a good idea!

@knqyf263 knqyf263 added this pull request to the merge queue Jul 9, 2024
Merged via the queue into aquasecurity:main with commit 7cbdb0a Jul 9, 2024
17 checks passed
@aqua-bot aqua-bot mentioned this pull request Jul 9, 2024
@DmitriyLewen DmitriyLewen deleted the refactor/pkg-types branch July 9, 2024 08:28
skahn007gl pushed a commit to skahn007gl/trivy that referenced this pull request Jul 23, 2024
romulets added a commit to elastic/cloudbeat that referenced this pull request Aug 13, 2024
- Fix --vuln-type flag renamed into --pkg-types aquasecurity/trivy#7104; \n- Adopt package relationships aquasecurity/trivy#7237
romulets added a commit to romulets/cloudbeat that referenced this pull request Aug 13, 2024
- Fix --vuln-type flag renamed into --pkg-types aquasecurity/trivy#7104; \n- Adopt package relationships aquasecurity/trivy#7237
romulets added a commit to elastic/cloudbeat that referenced this pull request Aug 13, 2024
* Bump trivy to v0.49.1

* Bump trivy to v0.51.4
    - Fix registry version aquasecurity/trivy#6219; 
    - Fix replace zap with slog aquasecurity/trivy#6466;
        - The fix with slog used a zap to slog bridge (official from zap, but exp). It didn't have a license file, so I hardcoded a commit version that had; 
  - Adopt opts.Align() to validate options object;

* Bump trivy to v0.52.2

* Temp change the workflow trigger to test changes

* Free up space on runner

* Bump trivy to v0.53.0
  - Fix go clear cache aquasecurity/trivy#7010

* Bump trivy to v0.54.1
  - Fix --vuln-type flag renamed into --pkg-types aquasecurity/trivy#7104; 
  - Adopt package relationships aquasecurity/trivy#7237

* Rollback CI run on target

* Clean 'scan cache clean' code and add timeout to it
afsmeira added a commit to codacy/codacy-trivy that referenced this pull request Aug 29, 2024
github-actions bot pushed a commit to codacy/codacy-trivy that referenced this pull request Aug 29, 2024
…#83)

* chore(deps): bump github.com/aquasecurity/trivy from 0.53.0 to 0.54.1

Bumps [github.com/aquasecurity/trivy](https://github.com/aquasecurity/trivy) from 0.53.0 to 0.54.1.
- [Release notes](https://github.com/aquasecurity/trivy/releases)
- [Changelog](https://github.com/aquasecurity/trivy/blob/v0.54.1/CHANGELOG.md)
- [Commits](aquasecurity/trivy@v0.53.0...v0.54.1)

---
updated-dependencies:
- dependency-name: github.com/aquasecurity/trivy
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* fix: Use new flag for vulnerability type

See aquasecurity/trivy#7104

* bump: Trivy version for vulnerability DB updates

* fix: Specify types of dependencies to analyze

It's only necessary to specifiy these types when running the scanners directly, as we do.
When running Trivy via the command line it's not necessary.

See aquasecurity/trivy#7237

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: André Meira <andre.meira@codacy.com>
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.

use --vuln-type flag for Packages
2 participants