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 devcontainer, update test_requirements.txt #892

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

Conversation

arogozhnikov
Copy link
Contributor

@arogozhnikov arogozhnikov commented Aug 21, 2024

Motivation: makes easier to contribute.

Currently to test things locally I need to delete aws config, and manually change requirements for tests, create virtual env and setup pre-commit

I can confirm that after spinning up the container, I can run pytest s3fs and it works:

========================================== 313 passed, 1 skipped, 3 xfailed, 12 warnings in 150.68s (0:02:30) ==========================================

@martindurant
Copy link
Member

How is this executed? Is this docker? I would say that requiring docker is actually more onerous. Simpler would be a pytest fixture that masks any environment variables that were creating trouble for you.

@arogozhnikov
Copy link
Contributor Author

docker or podman-as-docker if you run it on your machine.

For me that's this button:

Screenshot 2024-08-21 at 11 04 12

@arogozhnikov
Copy link
Contributor Author

For local development, pycharm and vs code support devcontainers now and start/manage container for you.

@arogozhnikov
Copy link
Contributor Author

For me that's this button:

you need devcontainer.json first to make button work correctly.

@martindurant
Copy link
Member

So you still need docker, and also one of the particular IDEs that supports this mechanism. I am mildly against the idea.

@arogozhnikov
Copy link
Contributor Author

up to you of course.

I'd spend 10 minutes setting this up rather ~3 hours figuring out why tests didn't work on mac or on linux, but worked in CI

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.

2 participants