Add Action input to set CodeQL DB location - #505
Conversation
|
|
📚 Thanks for the docs ping! 🛎️ This was added to our docs first-responder project board. A team member will be along shortly to respond. To request changes to the docs you can also open a |
@edoardopirovano - thanks for the docs team ping. 🙇🏻♀️ I think this will be easier to discuss and track in the related issue, so I've added that to our first responder board instead. |
aeisenberg
left a comment
There was a problem hiding this comment.
I think this looks fine. Two small suggestions that are not necessary to address.
| export function getCodeQLDatabasePath(dbDir: string, language: Language) { | ||
| return path.resolve(dbDir, language); |
There was a problem hiding this comment.
Nit: This is a small function. Would it make sense to inline?
There was a problem hiding this comment.
I think it's nice to have everywhere we resolve the location of a database flow through this function since it makes future changes to where databases are located easier.
There was a problem hiding this comment.
Also nit: Should this instead be taking a config, and then internally using config.dbLocation?
| } | ||
|
|
||
| let dbLocation; | ||
| if (DB_LOCATION in parsedYAML) { |
There was a problem hiding this comment.
The new field is in the CodeQL YAML config file. I was originally thinking this would be an Actions input to the init or analyze steps (see their action.yml files for the existing inputs), so that the option is directly settable from the Actions workflow YAML, and can reference other Actions context and environment variables.
I'm open to revising that though. What are the advantages of having this in the config file?
There was a problem hiding this comment.
Aha, adding it as an Actions input sounds totally reasonable given the above. I have no strong argument for putting it in the config file. I will change the option to an Actions input.
3cbc563 to
e6c7f81
Compare
e8e3f58 to
49185d3
Compare
adityasharad
left a comment
There was a problem hiding this comment.
Code looks reasonable, though I'll let Andrew have the final say.
Some comments mainly to ensure we have enough test coverage for both cases.
| export function getCodeQLDatabasePath(dbDir: string, language: Language) { | ||
| return path.resolve(dbDir, language); |
There was a problem hiding this comment.
Also nit: Should this instead be taking a config, and then internally using config.dbLocation?
| with: | ||
| db-location: "${{ runner.temp }}/customDbLocation" |
There was a problem hiding this comment.
Have we got a similar test where we use the default?
There was a problem hiding this comment.
All the other tests were using the default location anyway, but I've changed one of them to explicitly check that DBs are created there.
cb2777b to
8a9e1bb
Compare
8a9e1bb to
9b26313
Compare
aeisenberg
left a comment
There was a problem hiding this comment.
Nice work. My question is just curiosity and doesn't require any changes.
This adds a new option that lets the user control where the CodeQL databases that the Action creates are placed.
Documentation information
@github/docs-content-codeql Note that this documentation page will need updating: https://docs.github.com/en/code-security/secure-coding/configuring-code-scanning
The new option has the name
db-locationand should contain a path that the CodeQL Action can write to. If provided, this will result in the Action storing the CodeQL databases it creates at that location instead of the default temporary directory. A typical use case for this option would be that the user wishes for the CodeQL database to be in a consistent location so that a later step in their workflow can upload the DB somewhere.We should highlight that it is the responsibility of the user that if they set a non-temporary directory in this configuration option, they should ensure that they clear it in a step later in their workflow when they are done using the database.
Merge / deployment checklist