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: updating schema #3106

Merged
merged 2 commits into from
Jun 29, 2023
Merged

feat: updating schema #3106

merged 2 commits into from
Jun 29, 2023

Conversation

Rexbeast2
Copy link
Contributor

fixes: #3105

@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2023

Codecov Report

Merging #3106 (5907e37) into main (6d746c7) will decrease coverage by 0.89%.
The diff coverage is 52.38%.

@@            Coverage Diff             @@
##             main    #3106      +/-   ##
==========================================
- Coverage   82.46%   81.58%   -0.89%     
==========================================
  Files         714      714              
  Lines       10984    11000      +16     
  Branches     1477     1479       +2     
==========================================
- Hits         9058     8974      -84     
- Misses       1543     1625      +82     
- Partials      383      401      +18     
Flag Coverage Δ
longtests 81.04% <52.38%> (-0.18%) ⬇️
win-longtests 79.17% <52.38%> (-0.75%) ⬇️

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

Impacted Files Coverage Δ
cve_bin_tool/cvedb.py 64.08% <52.38%> (-4.42%) ⬇️

... and 4 files with indirect coverage changes

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


# Check schema on metrics
if not self.latest_schema("metrics", metrics_schema, cursor):
self.LOGGER.info("Upgrading cve_exploited data. This may take some time.")
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment in line 403/412 should be cve_metrics data, However, we aren't actually downloading the data at this stage, just creating the table

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is why I haven't included the download function. I have recently updated the table schema and made updates wherever it is referenced or utilized.

Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

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

I spent way too long staring at this wondering why you had a metrics table instead of an enum before I remembered that sqlite doesn't support enum. 🤦‍♀️ (I might be a little "hungover" from yesterday's migraine, sorry!)

I feel like we're going to have to refactor the actual schema update part before it turns into illegible gibberish with so many _ tossed in there, but I'm pretty sure that's my fault from the last revisions I did in there and not yours. In the interest of getting things merged before the weekend, I'm going to go ahead and get this merged now and we can refactor later if necessary. The failing tests don't appear to be related.

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.

feat: updating database schema
4 participants