-
Notifications
You must be signed in to change notification settings - Fork 193
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 origin metadata to database items #3066
Conversation
This adds a source property to the database item to store the source of the database, for example GitHub or an internet URL. This will be used to automatically check for updates to GitHub-downloaded databases in the future.
5d50d93
to
bca7ecb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of points I just want to clarify, but it seems ok.
@@ -88,6 +90,7 @@ export interface LocalDatabase { | |||
name: string; | |||
dateAdded: number; | |||
language: string; | |||
source: DatabaseSource; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea that this doesn't need to be an optional field because the UI currently doesn't allow you to add local databases to this config? This is the config for the new, combined databases panel, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. This type isn't actually used anywhere, but once we start using it, we should have the source for every database that is added. This is the same as the language
and dateAdded
, both of which are also optional in the FullDatabaseOptions
, but required in this type.
return { | ||
databaseUrl: `https://api.github.com/repos/${owner}/${repo}/code-scanning/codeql/databases/${language}`, | ||
owner, | ||
name: repo, | ||
commitOid: databaseForLanguage.commit_oid, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth noting that this is the commit oid at the point of checking the database info and it might have changed by the point that we download the database contents. However the download should happen immediately and I'm not sure we can do better anyway since there isn't an API endpoint that returns both the metadata and the contents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something we are aware of. The chance of this race condition happening is unlikely, but if it does, it's also not problematic. The only reason the commit_oid
will be used is for checking updates to the database. If we run into this race condition, it's probably pretty likely that it's an active repository and we would have new commits coming in shortly after anyways.
Passing suggestion: if you're concerned about the potential confusion around source or provenance, perhaps "origin"? |
Thanks! I think that's a better name for this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This adds an origin property to the database item to store the source of the database, for example GitHub or an internet URL. This will be used to automatically check for updates to GitHub-downloaded databases in the future.
Checklist
ready-for-doc-review
label there.