-
Notifications
You must be signed in to change notification settings - Fork 25
ci: update requirements.txt for snyk #692
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
base: main
Are you sure you want to change the base?
Conversation
|
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
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 think we can uv pip compile
to generate the requirements file.
Sure, we could, but that might entail bigger build tooling changes that I wasn't looking to take on myself right now. I was just hoping to clean up some nuisance snyk alerts. It turns out though that the comment that "requirements.txt is just used by snyk" is (no longer) true, it's referenced in several places in the repo, and maybe it's connected to why the integration tests failed, I haven't looked into it. So I'm going to have to rethink this. |
@edavidaja I edited the snyk workflow to |
I think #536 is the thing that needs to be unwound. [edit: or not, can't tell that this is used in CI, though we should address this anyway, perhaps by deleting] |
Tests are still failing but the only non-comment change in this PR is in snyk.yml so I don't see how they're related. |
Intent
The requirements.txt claims only exists to facilitate dependency scanning. We're getting spurious alerts from it. Fix that.
Approach
Add a reminder comment in pyproject.toml. Copy over to requirements.txt and delete the optional/dev dependencies listed there. Alphabetize.Update the
snyk.yml
workflow touv pip compile
frompyproject.toml
, so snyk stays in sync. Then delete therequirements.txt
because it's only used for snyk, right? Wrong, other things are failing. So we'll need to clean up other references to that requirements file.Automated Tests
CI should pass.