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

Bug: DockerImage path arg is misleading #610

Closed
black-snow opened this issue Jun 20, 2024 · 10 comments · Fixed by #615 or #613
Closed

Bug: DockerImage path arg is misleading #610

black-snow opened this issue Jun 20, 2024 · 10 comments · Fixed by #615 or #613

Comments

@black-snow
Copy link
Contributor

black-snow commented Jun 20, 2024

Describe the bug

testcontainers.core.image.DockerImage has a constructor arg path with the following docs:

path – Path to the Dockerfile to build the image

But actually this doesn't seem to be true. It's rather the context, the path to the directory to build in and the dockerfile itself must be named Dockerfile. Especially for testing this is very unhandy. I often have several dockerfiles in the same directory. I'd expect bein able to pass a context and the actual path to the dockerfile, like

with DockerImage(dockerfile="./caseA.Dockerfile", context=".", tag="casea:latest") as image:
     pass

To Reproduce

  • have a test.Dockerfile
  • with DockerImage(path="./test.Dockerfile", tag="test") as image: ...
  • see not too helpful error message from docker itself bubble up

Runtime environment

not relevant


P.S.: build-args would also be handy

@alexanderankin
Copy link
Member

so, that's a mistake - I'm pretty busy with my job these days but will review patches, seems like something to fix (and add re: build/args).

i might even argue for calling adding build/args a fix, as it makes our api more congruent with the docker api.

@black-snow
Copy link
Contributor Author

I'll take a look at it.

@black-snow
Copy link
Contributor Author

black-snow commented Jun 21, 2024

Err... make tests in the devcontainer leaves me with 32 failed, 42 passed, 3 warnings in 61.63s

/edit: locally they look better but you have to poetry install -E registry - only 26 tests fails then --- there should really be a readme for contributors

@alexanderankin
Copy link
Member

i think that makes sense if you are on windows. its on my list to find a piece of terraform code that creates a windows VM in aws, so i can develop against windows, lots of challenges there. (cc @kiview)

@alexanderankin
Copy link
Member

@black-snow you should be poetry install --with dev --all-extras if you are trying to run the full test suite (like pytest -s)

@black-snow
Copy link
Contributor Author

@alexanderankin I'm on Ubuntu 24 right now. Even with all the extras 30 tests still fail.

@black-snow
Copy link
Contributor Author

black-snow commented Jun 21, 2024

From https://docker-py.readthedocs.io/en/stable/images.html#docker.models.images.ImageCollection.build it's not very clear what exactly the path is either. Looking at the code I'm pretty sure this is not what I expect. So I might have to redirect this to docker-py first.

It's totally fine to docker build -t whatevs -f some/sub/dir/my.Dockerfile . - yet the code seems to require the dockerfile to be top-level in the path, which is treated as context.

/edit: okay dockerfile can be a path within path ... the naming seems quite unfortunate

@alexanderankin
Copy link
Member

@black-snow do you have your docker socket visible to your user? e.g. i think by default the docker socket has these permissions:

$ ll /var/run/docker.sock 
srw-rw---- 1 root docker 0 Jun 23 14:33 /var/run/docker.sock=

and then you need to be in the docker group:

$ groups | tr ' ' '\n' | cat -n | grep docker
    11	docker

@black-snow
Copy link
Contributor Author

black-snow commented Jun 27, 2024

@alexanderankin I have Docker Desktop on this machine an it keeps the sockets in ~/docker/desktop/docker.sock.

Never bothered to check on this machine - actually I'm not in the docker group here. But apparently this isn't necessary with recent Docker Desktop.

/edit /var/run/docker.sock seems to be baked in to some places

@alexanderankin
Copy link
Member

i opened #621 so that I can find this discussion easier later, never heard of docker desktop on linux before

alexanderankin pushed a commit that referenced this issue Jun 28, 2024
🤖 I have created a release *beep* *boop*
---


##
[4.7.0](testcontainers-v4.6.0...testcontainers-v4.7.0)
(2024-06-28)


### Features

* **core:** Added Generic module
([#612](#612))
([e575b28](e575b28))
* **core:** allow custom dockerfile path for image build and bypassing
build cache
([#615](#615))
([ead0f79](ead0f79)),
closes
[#610](#610)
* **core:** DockerCompose.stop now stops only services that it starts
(does not stop the other services)
([#620](#620))
([e711800](e711800))


### Bug Fixes

* **ollama:** Add support for ollama module
([#618](#618))
([5442d05](5442d05))
* **cosmosdb:** Add support for the CosmosDB Emulator
([#579](#579))
([8045a80](8045a80))
* improve ollama docs, s/ollama_dir/ollama_home/g
([#619](#619))
([27f2a6b](27f2a6b))
* **kafka:** Add Kraft to Kafka containers
([#611](#611))
([762d2a2](762d2a2))


### Documentation

* **contributing:** add contribution and new-container guide
([#460](#460))
([3519f4b](3519f4b))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants