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

Improve paths config handling for JS/Python when globs are used #2061

Closed
wants to merge 2 commits into from

Conversation

RasmusWL
Copy link
Member

@RasmusWL RasmusWL commented Jan 4, 2024

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@RasmusWL
Copy link
Member Author

RasmusWL commented Jan 4, 2024

I have only tested this change against the Python extractor implementation, where I know the ordering of LGTM_INDEX_FILTER is respected. I assume it would be by the other languages as well, but better be safe 😊

So I want to make an integration test to ensure that with a directory structure as below, a config with paths: ['include/foo*'], should only extract ONE file for Python (and likewise for JS/Ruby)

.
├── include
│   ├── bar.py
│   └── foo.py
└── top.py

or do you rather think that should be part of the tests for the extractors? (let me know what you think)

@henrymercer
Copy link
Contributor

Thanks for looking into this, this fix looks good. However I think we want to implement it in the CLI rather than the Action.

Context: We moved the logic for computing LGTM_INDEX_FILTERS into the CLI as of v2.11.6, the motivation being that we want this functionality to work for customers using third-party CI systems. Since then, we have had to maintain both the CLI implementation and a duplicate implementation in the Action so we can continue supporting older CLIs.

As of today, GHES 3.7 was deprecated, and therefore we've been able to drop support for CodeQL v2.11.5 and earlier in the Action. This means we no longer need to maintain the implementation of this functionality in the Action, and I've put up a PR to remove it here: #2062

Would you mind porting your implementation to the CLI? If not, I'm happy to take this on since it's a small change — let me know!

@RasmusWL
Copy link
Member Author

RasmusWL commented Jan 5, 2024

Makes sense. I'll close this PR then 👍

@RasmusWL RasmusWL closed this Jan 5, 2024
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.

None yet

2 participants