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

Add lock file #141

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Add lock file #141

wants to merge 11 commits into from

Conversation

shabeebk
Copy link

Vulnerability scanning tooling is using the lock file to look for issues. Without this, the tooling will not be able to produce reliable report.

Vulnerability scanning tooling is using the lock file to look for issues. Without this, the tooling will not be able to produce reliable report.
Pre commit hook helps the developers execute some tasks before committing.
@shabeebk shabeebk requested review from Aerylia and smite December 13, 2024 12:09
@Aerylia
Copy link
Contributor

Aerylia commented Dec 14, 2024

You’ll need to update the changelog. Minor release.

  • Added a requirements lock file that is automatically generated with tox and pre-commit

.pre-commit-config.yaml Outdated Show resolved Hide resolved
We are using python 3.11 in github check for requirements lock. Predictable version in the local precommit makes the CI job reliable.
@shabeebk
Copy link
Author

You’ll need to update the changelog. Minor release.

  • Added a requirements lock file that is automatically generated with tox and pre-commit

Added.

CHANGELOG.rst Outdated
Version 15.6
============

* Added requirements lock file that is automatically generated with tox and pre-commit.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requirements.txt seems not used anywhere?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or how is the vulnerability scanner configured to use the lockfile? Either way if it's only used by CI and not even included in the published python package I don't think a changelog is needed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked for one to track these changes in case some external dev runs into them. As we often have externals making PRs on this repo.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requirements.txt is used by the Aikido app integration and hence will not be visible here. And, the changelog is added as @Aerylia asked for it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is requirements.txt part of the published python package?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well perhaps it doesn't matter but I'd consider leaving it out to avoid confusion

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think, we should remove the changelog entry? I am positive about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it can be removed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted. We can squash the commits while merging to make it clean.

Copy link

@miikkako miikkako Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any benefit for having this lockfile if the python package and CI are not using it. I think we should either make the package depend on the lockfile - although for client libraries this wouldn't make much sense and would actually be a big nuisance for runtimes depending on this library - or simply ignore this aikido complaint which I think is correct for client library repos.

This reverts commit bae0a3e.
Copy link
Author

@shabeebk shabeebk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the lock file check in the CI.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
We cannot guarantee that the lock file remains same in the lifetime. Hence, removing the checks here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants