-
-
Notifications
You must be signed in to change notification settings - Fork 28
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 devpi docker image #67
Conversation
set +e | ||
tr -cd '[:alnum:]' < /dev/urandom | fold -w30 | head -n1 | tr -cd '[:alnum:]' | ||
set -e | ||
} |
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.
Maybe we could set some fixed password to make it simpler? Of in a CI env variable if you prefer to keep it a secret. We don't really care about it being random for testing.
|
||
DEVPI_ROOT_PASSWORD="${DEVPI_ROOT_PASSWORD:-}" | ||
if [ -f "$DEVPI_SERVER_ROOT/.root_password" ]; then | ||
DEVPI_ROOT_PASSWORD=$(cat "$DEVPI_SERVER_ROOT/.root_password") |
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.
All of this can be removed in our case probably. This script can just expect DEVPI_ROOT_PASSWORD
env variable to be set?.
} | ||
location /health | ||
{ | ||
return 200 '{"message": "healthy"}'; |
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.
What's the point of this endpoint, if it returns a constant anyway?
location /devpi/ | ||
{ | ||
proxy_pass http://devpi:3141/; | ||
add_header 'Access-Control-Allow-Origin' '*' always; |
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.
Can we open an issue at devpi to see if they can optionally add CORS ?
) | ||
def json_info_view(context, request) -> dict[str, Any]: | ||
# Override the base_url with the BASE_URL environment variable | ||
# This is useful when running behind a reverse proxy |
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.
Wait so the sha256 changes if run behind a reverse proxy? I would have guessed it shouldn't.
Isn't the point to use stock devpi server?
|
||
|
||
@pytest.fixture(scope="session") | ||
def devpi(docker_ip, docker_services): |
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.
IMO we should be able to run the test suite without relying on docker: so if would be better if this was opt-in.
Thanks @ryanking13! Some comments are above. I see the value of having a real server for Python wheels but I think Docker (or installing this server) ideally shouldn't be required to run tests (or at least it should be optional) As far as I know pip doesn't do this for tests. The other alternative is to use this test server, record the response and save them to the file system then mock the response with it. It's a bit more work though. Maybe @pradyunsg would have some guidance on the best way to test compliance with a server that exposes a PyPI-like JSON API? |
Yes, that is a good point. I was also a bit concerned about making docker mandatory to run tests. I like the idea of recording the responses from the test server and mocking the response with it. Basically, I wanted to remove our mock_fetch class, which is very hard to maintain I think. |
Closing as we added a way to run a mock server in #74. |
This PR adds devpi docker image, which is a private PyPI server that can be used for testing micropip.
devpi is not integrated with micropip tests yet, since micropip does not support alternative registries yet. I will open follow-up PRs.
This PR includes:
pytest-docker
package that integrates devpi docker with pytest