feat: Config generator #2699

Rexbeast2 · 2023-02-13T18:13:50Z

terriko

Looks like a good start, but I think we need to be more careful about the options we provide as "default" and I'd like to see it actually generating config for all available options, probably done programmatically from going through the same list that argparse uses in cli.py.

terriko · 2023-02-13T19:55:41Z

cve_bin_tool/cli.py

+            LOGGER.error(
+                f"Argument --generate-config: invalid choice: {config_type} (choose from 'toml','yaml')"
+            )
+            return -1


We had trouble with negative error codes on windows where they're not allowed (and it can give seemingly random numbers as a result) so we shouldn't return -1. Take a look at our existing set of error codes and see if we have a syntax error-y one that suits for this purpose, or make a new one, but definitely don't use -1.

terriko · 2023-02-13T19:57:58Z

cve_bin_tool/cli.py

+        elif config_type == "yaml":
+            config_generator.config_generate_yaml()
+    if args["generate_config"] != "":
+        return -1


Flagging this as another place where we need an error code other than -1.

terriko · 2023-02-13T19:59:06Z

cve_bin_tool/config_generator.py

+
+            input:
+              # Directory to scan
+              directory: test/assets


Probably don't actually want to set a scanning directory -- that could result in some pretty surprising behaviour. Maybe leave this line commented out?

Suggested change

directory: test/assets

# directory: test/assets

terriko · 2023-02-13T20:00:51Z

cve_bin_tool/config_generator.py

+
+            checker:
+              # list of checkers you want to skip
+              skips:


Ditto for skips and runs values: these would be surprising defaults; we probably want to have it commented out to show the syntax without changing behaviour.

terriko · 2023-02-13T20:02:08Z

cve_bin_tool/config_generator.py

+              # update schedule for NVD database (default: daily)
+              update: daily
+              # set true if you want to autoextract archive files. (default: true)
+              extract: true


Can we also auto-generate config from the other defaults? e.g. show that the default update method is 'nvd' but give options for json and stuff?

anthonyharrison

     if config_type not in available_type:
            LOGGER.info(config_type)
            LOGGER.error(
                f"Argument --generate-config: invalid choice: {config_type} (choose from 'toml','yaml')"
            )
            return 2
        elif config_type == "toml":
            config_generator.config_generate_toml()
        elif config_type == "yaml":
            config_generator.config_generate_yaml()

Do you need the config_type not in available_type check ? You already have a set of options in the --generate-config option which only allows valid options to be specified.

anthonyharrison · 2023-02-16T12:04:24Z

We need some tests adding.

Rexbeast2 · 2023-02-17T19:03:54Z

ok I will add test cases.

codecov-commenter · 2023-02-17T19:09:35Z

Codecov Report

Merging #2699 (c49bff5) into main (23b25e1) will increase coverage by 2.54%.
The diff coverage is 36.52%.

@@            Coverage Diff             @@
##             main    #2699      +/-   ##
==========================================
+ Coverage   79.36%   81.90%   +2.54%     
==========================================
  Files         646      649       +3     
  Lines        9909    10209     +300     
  Branches     1138     1377     +239     
==========================================
+ Hits         7864     8362     +498     
+ Misses       1681     1500     -181     
+ Partials      364      347      -17

Flag	Coverage Δ
longtests	`80.74% <36.52%> (?)`
win-longtests	`79.04% <36.52%> (-0.32%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cve_bin_tool/cvedb.py	`55.65% <15.25%> (-11.52%)`	⬇️
cve_bin_tool/cli.py	`68.27% <64.70%> (+1.39%)`	⬆️
cve_bin_tool/config_generator.py	`100.00% <100.00%> (ø)`
test/test_cli.py	`86.94% <100.00%> (+2.88%)`	⬆️

... and 21 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Rexbeast2 · 2023-02-22T17:09:33Z

@anthonyharrison I have added the test cases can you review them.

anthonyharrison

I think the tests need to be a bit more extensive. The tests just prove that a file is created and checks for the NVD key is in the file. I think we need to check all of the other parameters to ensure that they match the specified values.

It might be useful to allow the config filename to be specified rather than assuming that the file is always called config.toml or config.yaml.

I was also assuming that the config file would store any config values specified on the command line so that cve-bin-tool --offline --generate-config .... would set the offline flag in the config file. This doesn't sem to be done - I think the args array just needs to be passed to the generate functions for this to be done.

Rexbeast2 · 2023-02-23T10:11:31Z

@anthonyharrison I would add the feature where it parses args and adds it to the config file. Regarding testing, I created a check to ensure that the default file generated by the function config_generator works correctly. Additionally, the current implementation only checks for file names "config.toml" or "config.yaml" since those are the names of the default files generated.

making config generator dynamic

updating test for config generator

calling function with parameters

Rexbeast2 · 2023-03-06T17:53:13Z

@anthonyharrison @terriko I have updated the generate config function and test case. Can you review them?

anthonyharrison

@Rexbeast2 This is getting there. However, there is a lot of repeated information in the two file generators and I wonder if there could be a way of optimising it to create a single generator with two simple configuration routitnes -one for a heading and one for the parameters (the only difference for a parameter is whether ithe name is separated from the value by a : or =).. One of the issues we have is that as we add paramters to the tool, we will have to update both generators and it would be good if we only had one to update one as the parameters in the file are 'hard-coded' and it will be easy to miss an update.

anthonyharrison

@Rexbeast2 The test probably needs to also test some of the default values as well as those specified on the command line.

Rexbeast2 · 2023-03-06T21:28:59Z

@anthonyharrison thanks for the review, I will implement your suggestion and update the PR.

Rexbeast2 · 2023-03-07T22:14:03Z

@anthonyharrison I have updated the test cases and config generator as well. If there is more that could be improved let me know.

anthonyharrison

The code

        if config_type == "toml":
            config_generator.config_generator(args, config_type)
        elif config_type == "yaml":
            config_generator.config_generator(args, config_type)

can be simplified to

config_generator.config_generator(args, config_type)

In the config_generator, you may want to check that the config_type is only yaml or toml - the code will have undefined variables if the config type isn't valid.

I can't spot any ommisions, but it might be an interesting test to check that all of the elements in the args parameter are included. Maybe add a debug statement to the generator?

terriko · 2023-03-10T01:31:51Z

This is looking closer. It's probably not a perfect solution yet but it might be close enough to merge it and then iterate as needed. I'm going to update the branch and re-run the windows tests that were failing before doing a more careful code review. (it might not happen until next week)

…to config

terriko

Okay, this is still a bit more static than I'd like, but I think it's good enough that we should merge it and iterate from it in the tree. Thanks for working on this one, I know it took quite a few iterations already!

* fixes intel#2573

feat: Config generator

d7a4bd0

terriko requested changes Feb 13, 2023

View reviewed changes

Rexbeast2 added 2 commits February 14, 2023 17:56

Merge branch 'main' into config

0ea448e

fix: return statement

0caaef6

anthonyharrison reviewed Feb 16, 2023

View reviewed changes

fix: removing config_type

666b53a

Rexbeast2 added 2 commits February 21, 2023 11:05

Merge branch 'main' into config

f4bf459

feat: test case for config generator

f2d4192

anthonyharrison requested changes Feb 23, 2023

View reviewed changes

Rexbeast2 added 3 commits March 6, 2023 21:58

Update config_generator.py

fbdb61c

making config generator dynamic

Update test_cli.py

eee218c

updating test for config generator

Update cli.py

0799ee3

calling function with parameters

fix: pre-commit error

b719bcf

anthonyharrison reviewed Mar 6, 2023

View reviewed changes

anthonyharrison requested changes Mar 6, 2023

View reviewed changes

fix: improved and generalized config generator

b7b5a65

anthonyharrison requested changes Mar 9, 2023

View reviewed changes

fix: resolve conflict

55162b1

Merge branch 'main' into config

cf4bfd5

terriko added awaiting maintainer Need a maintainer to respond / help out awaiting CI labels Mar 10, 2023

Rexbeast2 added 7 commits March 10, 2023 11:33

fix: improve config

dcab36b

Merge branch 'main' into config

84135b2

fix: improving function calling

e1a79cf

Merge branch 'config' of https://github.com/Rexbeast2/cve-bin-tool in…

e05fd87

…to config

fix: fixing error

346ae88

fix: fixing error

2beb7a2

fix: fixing else statement

c49bff5

terriko approved these changes Mar 14, 2023

View reviewed changes

terriko merged commit 59bade4 into intel:main Mar 14, 2023

terriko mentioned this pull request Mar 14, 2023

Making the config generator more robust to config option changes #2821

Closed

terriko pushed a commit to anthonyharrison/cve-bin-tool that referenced this pull request Mar 30, 2023

feat: Config generator (intel#2699)

fdc209b

* fixes intel#2573

feat: Config generator #2699

feat: Config generator #2699

Uh oh!

Conversation

Rexbeast2 commented Feb 13, 2023

Uh oh!

terriko left a comment

Choose a reason for hiding this comment

Uh oh!

terriko Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

terriko Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

terriko Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

terriko Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

terriko Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

anthonyharrison left a comment

Choose a reason for hiding this comment

Uh oh!

anthonyharrison commented Feb 16, 2023

Uh oh!

Rexbeast2 commented Feb 17, 2023

Uh oh!

codecov-commenter commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Rexbeast2 commented Feb 22, 2023

Uh oh!

anthonyharrison left a comment

Choose a reason for hiding this comment

Uh oh!

Rexbeast2 commented Feb 23, 2023

Uh oh!

Rexbeast2 commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anthonyharrison left a comment

Choose a reason for hiding this comment

Uh oh!

anthonyharrison left a comment

Choose a reason for hiding this comment

Uh oh!

Rexbeast2 commented Mar 6, 2023

Uh oh!

Rexbeast2 commented Mar 7, 2023

Uh oh!

anthonyharrison left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terriko commented Mar 10, 2023

Uh oh!

terriko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Feb 17, 2023 •

edited

Loading

Rexbeast2 commented Mar 6, 2023 •

edited

Loading

anthonyharrison left a comment •

edited

Loading