GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Use the combined bundle when queries may need other languages' library packs by henrymercer · Pull Request #4184 · github/codeql-action Conversation
|
| File | Description |
|---|---|
src/per-language-bundles.ts |
Adds query eligibility logic. |
src/per-language-bundles.test.ts |
Tests eligibility and explanations. |
src/setup-codeql.ts |
Applies eligibility to release and nightly bundles. |
src/setup-codeql.test.ts |
Tests combined-bundle selection. |
src/setup-codeql-action.ts |
Forces combined bundles for standalone setup. |
src/init-action.ts |
Collects query configuration inputs. |
src/init.ts |
Propagates bundle-selection reasoning. |
src/codeql.ts |
Propagates setup options. |
src/codeql.test.ts |
Updates setup calls. |
src/upload-lib.ts |
Updates initialization call. |
src/config/db-config.ts |
Centralizes built-in suite names. |
src/analyze.ts |
Uses centralized suite names. |
src/analyze.test.ts |
Updates suite import. |
setup-codeql/action.yml |
Corrects input documentation. |
pr-checks/checks/export-file-baseline-information.yml |
Disables per-language bundles for the baseline check. |
lib/entry-points.js |
Generated output; excluded from review. |
.github/workflows/__export-file-baseline-information.yml |
Generated workflow; excluded from review. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking care of this! I think this approach makes sense to work around the issue. Query customisation is reasonably rare so that this still allows most users to benefit from per-language bundles, while avoiding any issues like we saw in CI for those that do customise them.
I left a few comments, with one or two points about long-term maintainability and otherwise minor comments. So, generally this looks pretty good already and it shouldn't be far off from being ready to merge.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Copilot App <[email protected]>
…ry property Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
…cked Co-authored-by: Copilot App <[email protected]>
…lows Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing the comments from my previous review! I've had some more comments on the changes here. The main ones are again related to the maintainability of duplicated logic and whether we can reduce that or move some of the work to a common place before any of the consumers.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| const query = findNonBuiltInQuery( | ||
| parseQueriesInput(inputs.queriesInput).input, | ||
| ); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for changing this to better reuse the existing implementation!
Another concern here, which I don't think is blocking for this PR, is that getOtherLanguagePacksReason is more conservative than it needs to be. Specifically, we ignore the combines property and so are ignoring the precedence / combination rules that are implemented in combineQueries. Since that may discard some of the configured queries if others take precedence / don't allow combining, not all of the configured queries may actually end up getting passed to the CLI. If that is the case, then we might disable per-language bundles here even though we may not need to. Since this is more conservative than necessary, it is fine as-is, but we could look into reusing more of the combineQueries logic here to ensure consistency and to allow per-language bundles in more cases.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I agree that we are more conservative than necessary, which is fine, and that it would make sense to look into refactoring this as future work.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Copilot App <[email protected]>
…fig` input Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
… check Co-authored-by: Copilot App <[email protected]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
A bunch more comments on this, sorry. There's nothing critical, but just a bunch of instances where comments are worded poorly or odd changes were made as part of a refactoring.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
…is known to send Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
…pper Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
…put test Co-authored-by: Copilot App <[email protected]>
…isely Co-authored-by: Copilot App <[email protected]>
Co-authored-by: Copilot App <[email protected]>
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I think all of my feedback is addressed now and I don't have anything to add on the most recent set of commits.
Happy to approve on that basis. I haven't re-reviewed the full changes. If there's anything that you'd like me to take another look over, let me know. Otherwise, feel free to go ahead and merge this!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.