-
Notifications
You must be signed in to change notification settings - Fork 7
Fix issues with Docker #27
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
Conversation
|
@dedeswim All tests pass! But I would recommend you add tests specific to the functionality you found buggy as well. I agree, hex strings are a pain and it would be good to have those tests in case we implement different docker clients with different ways to pass arguments or files to and from the container. |
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.
@dedeswim All tests pass! But I would recommend you add tests specific to the functionality you found buggy as well. I agree, hex strings are a pain and it would be good to have those tests in case we implement different docker clients with different ways to pass arguments or files to and from the container.
|
@evtimovi I am not really sure we can add tests for this as this behavior was specific for Docker and I don't really know how we can test this? Also, these are specific to tasks, and the existing tests are exactly how I found about this issue. It's not a general thing for Siren I believe |
|
@dedeswim Hmm, it seems like the more general issue here that is not specific to tasks is this (from a comment in the changes): So we could add tests where we need to pass multi-line heredocs to simple/dummy Dockerfiles, no? |
evtimovi
left a comment
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.
From a side conversation it sounds like the existing tests were already failing here with Docker before this test, so let's merge!
|
fyi @evtimovi I can't merge this myself. I think you have to do it |
|
Ahh sorry - I thought approving was enough to let you merge! Done now! |
This PR fixes some issues that I encountered when running SWE-bench using Docker instead of Podman, as it looks like Docker struggles with heredocs within
RUNsteps. So we encode the content to be written to file to base64 (and decode it before writing).@evtimovi can you please try to run the integration tests with Podman? On my own machine I have Docker so I can't test it