Skip to content

Add devenv-requirements.txt and update env setup instructions - #2761

Merged
antonpirker merged 2 commits into
getsentry:masterfrom
arr-ee:devenv-requirements
Apr 4, 2024
Merged

Add devenv-requirements.txt and update env setup instructions#2761
antonpirker merged 2 commits into
getsentry:masterfrom
arr-ee:devenv-requirements

Conversation

@arr-ee

@arr-ee arr-ee commented Feb 25, 2024

Copy link
Copy Markdown
Contributor

This is a quick fix for #2760.

Ideally we'd do away with constraints duplication/introduction in tox.ini, but I don't see a way to do so without much more invasive changes, and I would prefer to not make those before discussing approaches.

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.

@antonpirker

antonpirker commented Mar 28, 2024

Copy link
Copy Markdown
Contributor

Hey @arr-ee !
First, sorry for taking so long for reviewing this.

Thank you so much for this PR! Our devenv needs really some love :-)

One thing, there is also a mention of linter-requirements.txt in tox.ini. (which is run when you run make lint on your local machine. Could you update your PR so this still works? Thanks!

@antonpirker antonpirker self-assigned this Mar 28, 2024
@arr-ee
Add devenv-requirements.txt and update env setup instructions
95a6403 
This is a quick fix for getsentry#2760.

It's far from ideal, but it's quick.
@arr-ee

arr-ee commented Apr 2, 2024

Copy link
Copy Markdown
Contributor Author

One thing, there is also a mention of linter-requirements.txt in tox.ini. (which is run when you run make lint on your local machine. Could you update your PR so this still works? Thanks!

This PR does not remove linter-requirements.txt. devenv-requirements.txt just installs both test and linter deps, as well as adding constraints on pytest & friends, and packages required for integration tests that get enabled by linter requirements (mongo). tox should not be affected by this change in any way.

@arr-ee
arr-ee force-pushed the devenv-requirements branch from d0fc8e8 to 95a6403 Compare April 2, 2024 14:27
@arr-ee

arr-ee commented Apr 2, 2024

Copy link
Copy Markdown
Contributor Author

db test failure is unrelated:

[17](https://github.com/getsentry/sentry-python/actions/runs/8524540156/job/23349361343#step:6:18)
Unable to find image 'clickhouse/clickhouse-server:latest' locally
[18](https://github.com/getsentry/sentry-python/actions/runs/8524540156/job/23349361343#step:6:19)
docker: Error response from daemon: received unexpected HTTP status: 503 Service Unavailable.

@antonpirker

antonpirker commented Apr 4, 2024

Copy link
Copy Markdown
Contributor

Thanks for the clarification, makes sense :-)

As soon as the flaky test passes, this will be merged! Thanks again @arr-ee !

@antonpirker
antonpirker enabled auto-merge (squash) April 4, 2024 09:18
@antonpirker
antonpirker merged commit 539412c into getsentry:master Apr 4, 2024
@szokeasaurusrex szokeasaurusrex mentioned this pull request Jun 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@antonpirker antonpirker antonpirker approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@antonpirker antonpirker

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@arr-ee @antonpirker