Skip to content

refactor(cli): add custom flag groups #8594

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

knqyf263
Copy link
Collaborator

@knqyf263 knqyf263 commented Mar 21, 2025

Description

This PR introduces a new extension system to support custom CLI flag groups for various Trivy commands. The implementation:

  • Creates a new mechanism that allows extensions to define their own flag groups
  • Adds a CustomFlagGroups function that collects flag groups from registered extensions
  • Updates all major commands to include custom flag groups from extensions
  • Adds CustomOptions map to the flag options to store custom values
  • Provides test coverage to validate the extension behavior and demonstrate the usage

This enhancement improves modularity and flexibility by allowing extensions to define their own CLI flags without modifying the core codebase. This is primarily an internal change for Aqua Security.

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 self-assigned this Mar 21, 2025
@knqyf263 knqyf263 force-pushed the refactor/custom_flags branch from 14b4f69 to 39470c7 Compare March 21, 2025 13:16
@itaysk
Copy link
Contributor

itaysk commented Mar 22, 2025

can you confirm if it's possible to add a flag to an existing flag group, it it has to be a new one?

@knqyf263
Copy link
Collaborator Author

knqyf263 commented Mar 23, 2025

can you confirm if it's possible to add a flag to an existing flag group, it it has to be a new one?

This PR does not support adding flags to existing flag groups. It supports additional flag groups. If we need to extend flags in the production version, I think it's sufficient to group flags that are only valid in the production version into a dedicated flag group for production. As far as I know, this approach meets the requirements, but if adding to existing groups is mandatory, the implementation will need to be adjusted.

Base automatically changed from feat/hook to main April 8, 2025 12:11
- Introduced a new extension system to support custom CLI flag groups for various commands.
- Updated command definitions to utilize the new `CustomFlagGroups` method, enhancing modularity and flexibility.
- Added tests to validate the behavior of custom flag groups and ensure proper integration with existing command structures.
- Improved overall flag handling by allowing extensions to define their own flag groups, facilitating future enhancements.
@knqyf263 knqyf263 force-pushed the refactor/custom_flags branch from ed62483 to 84eb623 Compare April 11, 2025 11:23
@danielciuraru86
Copy link

@knqyf263 looks good. Will this support setting the flag value both from the command and environment variable?
ex: trivy fs --my-custom-flag / TRIVY_MY_CUSTOM_FLAG?

IMO, changing an existing flag group is not a requirement from our side. @tamirkiviti13 wdyt?

…ble support

- Added a test case to verify the behavior of the `TRIVY_FOO` environment variable in the `TestCustomFlagGroups` function.
- Ensured that the flag system correctly retrieves values from both environment variables and custom flags.
- Improved test coverage for the flag handling functionality.
@knqyf263 knqyf263 force-pushed the refactor/custom_flags branch from fe64ab8 to 51cefc4 Compare April 14, 2025 09:08
@knqyf263
Copy link
Collaborator Author

@knqyf263 looks good. Will this support setting the flag value both from the command and environment variable?
ex: trivy fs --my-custom-flag / TRIVY_MY_CUSTOM_FLAG?

Yes, it's supported: 51cefc4

@tamirkiviti13
Copy link
Contributor

@danielciuraru86 @knqyf263 @itaysk
Regarding changing an existing flag group - it depends how you look at it.
For example, let's say I have an extension, called my-ext, that adds some functionality to the scan mechanism and requires a new flag.
Does the new flag belong to a new "My Ext Flags" group or does it belong to the existing "Scan Flags" group?

@itaysk
Copy link
Contributor

itaysk commented Apr 14, 2025

yes that was my thinking as well

@knqyf263
Copy link
Collaborator Author

Does the new flag belong to a new "My Ext Flags" group or does it belong to the existing "Scan Flags" group?

I don't mind either way, but since I thought that gathering flags available only in the enterprise version in one place would make them easier to view, I have currently implemented only the addition of custom flag groups. This also keeps the implementation simple. However, if there is a good reason to include them in the existing Scan Flags group, we can implement that.

@itaysk
Copy link
Contributor

itaysk commented Apr 14, 2025

the reason is better UX IMO. for example, let's assume --license-confidence-level was implemented as extension. It would have been best to add it to the existing license flag group, and not a new flag group.

@knqyf263
Copy link
Collaborator Author

These flags are currently displayed by default regardless of whether an Aqua token is passed. In other words, they will always be displayed when running trivy --help. I personally worry that mixing enterprise flags irrelevant to most users into the existing OSS flag group might lead to a degraded UX.

@knqyf263
Copy link
Collaborator Author

knqyf263 commented Apr 14, 2025

In other words, I think that for OSS users it would be better if they were grouped separately, while for enterprise users it may be preferable that they are merged into the existing flag group.

@tamirkiviti13
Copy link
Contributor

I tend to agree with @knqyf263, because the flags are always displayed.
Is it possible to create a subgroup mechanism? Or is it too much?

@itaysk
Copy link
Contributor

itaysk commented Apr 14, 2025

I do think we should display the extension flags always, not only to authenticated users, but with a label or indicator that they are differentiated (like some product uses the term "pro" features). if we group all "pro" features in a single seperate group (and I suspect we'll have quite a few of those new features) it wouldn't make much sense UX wise. Consider also that in the documentation they would be mixed, for example, to continue my earlier example the --license-confidence-level flag would be documented in general license scanner doc, along with it's other (oss) features.

@knqyf263
Copy link
Collaborator Author

if we group all "pro" features in a single seperate group (and I suspect we'll have quite a few of those new features) it wouldn't make much sense UX wise.

Why is that? For those who are interested in enterprise features, they can simply refer to the dedicated flag group to see what options are available. Therefore, having an Enterprise flag group is somewhat logical for me. I’d like to understand how you think it would impact the UX.

Consider also that in the documentation they would be mixed, for example, to continue my earlier example the --license-confidence-level flag would be documented in general license scanner doc, along with it's other (oss) features.

Does that mean that, from that perspective, it would be better to separate the Enterprise features from OSS features? Or are you suggesting that even in the documentation, mixing OSS and Enterprise features is preferable?

Additionally, I attempted to write a PoC, but the implementation cost turned out to be higher than expected. Once again, considering that mixing Enterprise flags into the existing flag group might negatively impact the user experience for OSS users, I’m not sure whether the benefits of incurring that implementation cost would be worthwhile for most Trivy users.

I would like to understand whether the decision is based on accepting a degraded UX for OSS users if it improves UX for Enterprise users or if the belief is that mixing the flags would yield a better UX for OSS users as well.

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.

4 participants