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

build: Remove ipdb from test requirements #3237

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

rominf
Copy link
Contributor

@rominf rominf commented Jul 3, 2024

ipdb is not used by testing suite. To avoid installing extra dependencies, remove it from requirements file. Developers who find ipdb helpful can install the package themselves.

The change was initially driven by the fact that ipdb was not installable on Fedora 41 because of its missing Python 3.13 support, which made the Sentry SDK build fail due to unsatisfied build requirements.


General Notes

Thank you for contributing to sentry-python!

Please add tests to validate your changes, and lint your code using tox -e linters.

Running the test suite on your PR might require maintainer approval. Some tests (AWS Lambda) additionally require a maintainer to add a special label to run and will fail if the label is not present.

For maintainers

Sensitive test suites require maintainer review to ensure that tests do not compromise our secrets. This review must be repeated after any code revisions.

Before running sensitive test suites, please carefully check the PR. Then, apply the Trigger: tests using secrets label. The label will be removed after any code changes to enforce our policy requiring maintainers to review all code revisions before running sensitive tests.

@szokeasaurusrex
Copy link
Member

szokeasaurusrex commented Jul 3, 2024

Thank you for the contribution! I would suggest that we simply remove the ipdb dependency completely, unless there is some reason you think it makes sense to add to requirements-devenv?

[ipdb](https://pypi.org/project/ipdb) is not used by testing suite. To
avoid installing extra dependencies, remove it from requirements file.
Developers who find ipdb helpful can install the package themselves.
@rominf rominf changed the title Move ipdb to correct requirements file Remove ipdb from requirements file Jul 3, 2024
@rominf
Copy link
Contributor Author

rominf commented Jul 3, 2024

@szokeasaurusrex Thank you for your review. I agree! Done.

@szokeasaurusrex szokeasaurusrex added the Trigger: tests using secrets PR code is safe; run CI label Jul 3, 2024
Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

Thanks for making the change, looks good!

@szokeasaurusrex szokeasaurusrex changed the title Remove ipdb from requirements file build: Remove ipdb from test requirements Jul 3, 2024
@rominf
Copy link
Contributor Author

rominf commented Jul 3, 2024

@szokeasaurusrex I noticed your change in the PR title. I agree, this looks better. I can change commit message accordingly. Should I or you do not want extra CI runs?

@szokeasaurusrex
Copy link
Member

@rominf We usually use "Squash and merge" to merge PRs to master, and I am able to edit the commit message used in the merge. I was already planning to update the commit message accordingly, as that is the easiest way to fix the commit message (no CI rerun needed).

But, thanks for checking!

@szokeasaurusrex szokeasaurusrex merged commit defb448 into getsentry:master Jul 3, 2024
123 of 125 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Trigger: tests using secrets PR code is safe; run CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants