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

Add grype db providers command #2174

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add grype db providers command #2174

wants to merge 9 commits into from

Conversation

ADorigi
Copy link

@ADorigi ADorigi commented Oct 9, 2024

Issue

resolves #2131

Changes

  • Provider data is read from provider-metadata.json file.
  • Added flag -o/--output flags which accept json and table.
  • Display logic is separate from the provider data collection.
  • Can be easily updated for db schema v6.
  • Fails gracefully, with relevant error message, for absence of provider-metadata.json file.
  • Updated Readme.md for the new command.

- currently reads content of `provider-metadata.json` file
- added flag `-o`/`--output` flags which accept `json` and `table`
- update  method `getDBProviders()` and type `dbProviderMetadata` for db schema `v6`

Signed-off-by: Adnan Gulegulzar <[email protected]>
@ADorigi ADorigi marked this pull request as ready for review October 9, 2024 19:57
@willmurphyscode
Copy link
Contributor

Hi @ADorigi thanks for the quick work here!

We need a couple things before we can merge it.

  1. Can you clean up static analysis so that it passes? I think running make lint-fix and committing the resulting diff is all that's needed. You can check locally whether make static-analysis passes - that's the same check as runs in CI.
  2. Can you add a test for this command? You could either add a unit test as a sibling of your new file, or a CLI test in https://github.com/anchore/grype/tree/main/test/cli that proves the new command exists and prints the right sort of thing.

Let me know if you have any questions! Thanks!

@willmurphyscode willmurphyscode self-assigned this Oct 10, 2024
Signed-off-by: Adnan Gulegulzar <[email protected]>
@ADorigi
Copy link
Author

ADorigi commented Oct 16, 2024

@willmurphyscode
I have addressed the following:

  1. Cleaned the branch with make lint-fix and make static-analysis, ensuring it passes.
  2. Added a test for the grype db providers command in the test/cli directory.

@willmurphyscode
Copy link
Contributor

Hi @ADorigi! Thanks for getting up a rev that passes the validations. I have being trying out the command locally, and I have some additional feedback. There are some things that need to change in order to merge this, and some things that are merely nice to have, listed separately here:

Things that need to change

Nice to have changes:

  • nitpick: internal.TableOutput etc. constants are too broadly scoped - I realize that you did this to make the linter stop complaining about duplicate string literals, but db diff and db providers are totally different, so they should really each have their own constants.
  • For extra credit: create some exemplary unit tests in cmd/grype/cli/commands/db_providers.go. I realize there aren't great examples to work from, but if you could get a small table test going that checks on different outputs, you could move cases besides -o json into unit tests and keep the CLI tests small.

ADorigi and others added 2 commits October 21, 2024 20:29
- updated table as the default output format
- updated tablewriter settings
- added unit test for the components of db providers command
- added dummy "provider-metadata.json" to aid unit tests
- added table and json assertion to cli test

Signed-off-by: Adnan Gulegulzar <[email protected]>
@ADorigi
Copy link
Author

ADorigi commented Oct 22, 2024

Hello @willmurphyscode
Thank you for explaining the required changes in detail. Greatly appreciate the same.

I have made the following changes:

  • Updated the default output format to be table, to align it with the other commands.
  • Updated the formatting of the tablewriter to match the one in syft.
  • The db providers command supports only table and json as the output formats, with table now as default.
  • Added json and table trait assertions for the cli tests.
  • Limited the use of constants to only the db providers command
  • Updated test files
    • added a test file alongside cmd/grype/cli/commands/db_providers.go, to test individual components of the implementation
    • tests in test/cli/db_providers_test.go deal with how the command will actually be used.
    • as mentioned, I have not used any concrete example for these tests and will be willing to update them if required.

@ADorigi ADorigi reopened this Oct 22, 2024
Signed-off-by: Adnan Gulegulzar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

Add DB providers command
2 participants