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 CustomContainer exposing settings through the class #238

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

Conversation

vikahl
Copy link

@vikahl vikahl commented Aug 25, 2022

Add a CustomContainer exposing the settings through the class.

Close #236

@vikahl vikahl changed the title Add pyenv file to gitignore Add CustomContainer exposing settings through the class Aug 25, 2022
@vikahl vikahl marked this pull request as ready for review August 25, 2022 13:34
@tillahoffmann
Copy link
Collaborator

Great, thank you! Slight misunderstanding: I was thinking that we could add this functionality to existing containers rather than create a new one. What do you think?

@vikahl
Copy link
Author

vikahl commented Aug 25, 2022

Great, thank you! Slight misunderstanding: I was thinking that we could add this functionality to existing containers rather than create a new one. What do you think?

I don't there is any current class (except for the deprecated TestContainer, but probably best to not touch it) that allows specifying the image name. I build the service image prior to tests so I have e.g., my-service:version1 as an image and want to manually specify that.

@tillahoffmann
Copy link
Collaborator

The image name can be specified for DockerContainer here.

class DockerContainer(object):
def __init__(self, image, docker_client_kw: dict = None, **kwargs):

@vikahl
Copy link
Author

vikahl commented Aug 25, 2022

The image name can be specified for DockerContainer here.

class DockerContainer(object):
def __init__(self, image, docker_client_kw: dict = None, **kwargs):

True, you have a point there.

Just to clarify, do you suggest extending the __init__ method of DockerContainer or all the db-containers?

I think the DockerContainer approach would be convenient and won't require a new class. I can test the approach tomorrow and see if it would impact existing implementations.

@tillahoffmann
Copy link
Collaborator

Just to clarify, do you suggest extending the init method of DockerContainer or all the db-containers?

I was thinking all the containers as they all inherit from DockerContainer. That would give us a consistent interface across the whole library.

@vikahl
Copy link
Author

vikahl commented Mar 16, 2023

Just to clarify, do you suggest extending the init method of DockerContainer or all the db-containers?

I was thinking all the containers as they all inherit from DockerContainer. That would give us a consistent interface across the whole library.

Ah, good thought. Sorry for not picking up on this but I'll update the PR.

@vikahl
Copy link
Author

vikahl commented Mar 22, 2023

@tillahoffmann updated the PR to instead set these in the core container's __init__. Do you think this looks reasonable?

If so, I'll make sure these changes are covered by tests as well.

@gaby
Copy link

gaby commented Jun 8, 2023

Ping @tillahoffmann this is another great PR

Copy link
Collaborator

@tillahoffmann tillahoffmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I left a few small comments. If you could add tests, that'd be great! 🙏

core/testcontainers/core/container.py Outdated Show resolved Hide resolved
core/testcontainers/core/container.py Outdated Show resolved Hide resolved
@vikahl
Copy link
Author

vikahl commented Jun 9, 2023

Will take a look and add tests and do the updates when I am back at a computer (might take a few weeks)

Add test that tests that attributes can be set through the __init__
function and later read through class attr.

This test does not test that the attributes has an actual effect, in the
future it might be of interest to test the integration to the docker
container and ensure that class attributes are propagated properly.
@vikahl
Copy link
Author

vikahl commented Jun 27, 2023

@tillahoffmann please take a look at the tests.

I added test that tests that attributes can be set through the init function and later read through class attr.

The test does not test that the attributes has an actual effect, in the future it might be of interest to test the integration to the docker container and ensure that class attributes are propagated properly. However, I for the changes in this PR I think the current is good enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide generic DockerContainer with init attributes
3 participants