-
Notifications
You must be signed in to change notification settings - Fork 382
Overlay databases: Use Config
instead of AugmentationProperties
#3072
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
Conversation
d0ba608
to
029e76e
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.
Pull Request Overview
This PR refactors the overlay database configuration by moving related properties from the AugmentationProperties
interface to the main Config
interface for better organization and clarity.
Key Changes:
- Moves overlay database mode and caching properties from
AugmentationProperties
toConfig
- Adds
computedConfig
field to store the final computed configuration - Updates all references to use the new property locations
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/config-utils.ts | Moves overlay database properties from AugmentationProperties to Config , adds computedConfig field |
src/testing-utils.ts | Updates test configuration to use new property locations |
src/overlay-database-utils.ts | Updates overlay database utility functions to access properties from config instead of config.augmentationProperties |
src/init-action.ts | Updates initialization code to use new property structure |
src/analyze.ts | Updates analysis functions to use new property locations |
src/codeql.ts | Updates CodeQL functions to use new property structure and removes dependency on generateCodeScanningConfig |
src/test.ts | Updates test files to use new property structure |
lib/*.js | Generated JavaScript files with corresponding changes |
src/config-utils.ts
Outdated
if (config.computedConfig["query-filters"] === undefined) { | ||
config.computedConfig["query-filters"] = []; | ||
} | ||
config.computedConfig["query-filters"].push({ |
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.
We need to be a careful here. Ordering matters in query-filters
, and there was a recent bug where we accidentally overrode the user-specified query-filters
setting by putting our own exclusion at the top of the list. See comments at
codeql-action/src/config-utils.ts
Lines 1478 to 1485 in 68d7fe3
augmentedConfig["query-filters"] = [ | |
// Ordering matters. If the first filter is an inclusion, it implicitly | |
// excludes all queries that are not included. If it is an exclusion, | |
// it implicitly includes all queries that are not excluded. So user | |
// filters (if any) should always be first to preserve intent. | |
...(augmentedConfig["query-filters"] || []), | |
...augmentationProperties.extraQueryExclusions, | |
]; |
Saving the programmatic exclusions in extraQueryExclusions
allows us to easily ensure that those exclusions always come after user-specified filters. If we are mixing them all together in the same ``query-filters` field, we would need some other way to ensure proper ordering.
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.
Yep, I am aware that the ordering here is critical and that you previously fixed an issue related to that.
For context, the main motivation for this PR is that the overlay database settings don't really belong in AugmentationProperties
and we are working on removing the AugmentationProperties
in Config
.
In other words, the line you commented on does not produce a UserConfig
that's later combined with other AugmentationProperties
using generateCodeScanningConfig
. The UserConfig
that is mutated on the line you've commented on is the UserConfig
that is given to the CLI. Since we are appending exclude-from-incremental
here, it should have the same effect as what you previously accomplished by modifying AugmentationProperties
.
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.
First, thank you for the explanation!
There are two parts to my concern.
- That this PR preserves the existing behavior of putting the
exclude-from-incremental
exclusion after user-specified query filters - That we help future developers (which could well be our future selves) avoid introducing this regression by accident
You made a strong case that you addressed the first part, and I agree with you on that. What about the second part?
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.
Thanks for separating those points. I think what 2. has made me realise here is that you had introduced extraQueryExclusions
to AugmentationProperties
as part of your work on the Action in the last few months, but that it also doesn't really belong into AugmentationProperties
.
What I could propose then to address 2. is to move extraQueryExclusions
to Config
as well, add exclude-from-incremental
to it here, and then apply extraQueryExclusions
to the query-filters
of the UserConfig
that we pass on to the CLI as late as possible (i.e. just before invoking the CLI). That would then behave similarly to what we had previously, except we don't have the baggage of the entire AugmentationProperties
.
There might be more we could do to protect against shooting ourselves in the foot, but I think that's largely outside the scope of this change.
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.
I have pushed a commit which moves extraQueryExclusions
out of AugmentationProperties
into Config
. The part of the code from generateCodeScanningConfig
that appends the extraQueryExclusions
to a UserConfig
is now in its own function as well. I have added a bit more documentation at the point where we append the extra query exclusions that links to your previous PR as well.
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.
Thank you! These changes addressed all my concerns.
029e76e
to
ac9b91e
Compare
e4b1ce5
to
fe428a8
Compare
fe428a8
to
e9fb72d
Compare
…further modification
61c593a
to
7f81363
Compare
Extracted from #3064. Moves
extraQueryExclusions
and overlay database configuration out ofAugmentationProperties
and intoConfig
.The reasoning for this change is that
AugmentationProperties
is intended for user inputs to theinit
Action that cannot be represented in the same format that is used byUserConfig
(which is understood by the CodeQL CLI). Options stored in theAugmentationProperties
are then intended to be combined with an existingUserConfig
into a newUserConfig
.We will remove
AugmentationProperties
fromConfig
(used to represent the CodeQL Action configuration / state that is shared across different steps in a CodeQL workflow) in #3075. This makes sense, because the purpose ofAugmentationProperties
(as explained above) is just to represent user inputs to the Action until they can be converted into aUserConfig
.Risk assessment
For internal use only. Please select the risk level of this change:
Merge / deployment checklist