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 support for WithCreateContainerParametersModifier #503

Conversation

Xitric
Copy link
Contributor

@Xitric Xitric commented Jul 2, 2022

This PR adds a new builder method WithCreateContainerParametersModifier() which corresponds to withCreateContainerCmdModifier() from the Java implementation.

The purpose of this method is to gain access to low-level configuration on the Docker.DotNet client, for instance to configure Ulimits, without having to expose all of these low-level configurations through the testcontainer builder.

Merging this PR closes #481.

@HofmeisterAn
Copy link
Collaborator

I won't be able to do the review until the beginning of next week - sorry, but thanks already for your pull request and contribution.

Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I added some minor suggestions and improvements. Could you please cover your changes with a unit test in: DotNet.Testcontainers.Tests.Unit.Containers.Unix.TestcontainersContainerTest.WithConfiguration. Something like:

...
.WithCreateContainerParametersModifier(parameters = parameters.Name = "Foo")
.WithCreateContainerParametersModifier(parameters = parameters.Name = "Bar")
...

Assert("Bar", ...)

@Xitric
Copy link
Contributor Author

Xitric commented Jul 4, 2022

@HofmeisterAn Yes of course I should add a test. I looked around for a short while but I could not determine the right location for it. It has been added now. I just need clarification on a couple of your comments now

@Xitric
Copy link
Contributor Author

Xitric commented Jul 4, 2022

@HofmeisterAn There you go. Also added some unit tests for BuildConfiguration to clarify expectations of the different method implementations. That way we can catch it if someone breaks contract.

I have never used XUnit before, so pardon me if the class structure is not in line with best practices. Let me know if I should use a combination of the static outer class and sealed inner classes (I have seen that in other tests), and I can refactor it.

@HofmeisterAn HofmeisterAn force-pushed the feature/481-create-container-parameters-modifier branch from e7459aa to 2e4d020 Compare July 5, 2022 08:37
Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

If you like, you can squash the commits. I'll merge the pull request later that day, tomorrow at the latest.

@Xitric Xitric force-pushed the feature/481-create-container-parameters-modifier branch from 2e4d020 to 3cd6fb7 Compare July 5, 2022 10:50
@Xitric
Copy link
Contributor Author

Xitric commented Jul 5, 2022

@HofmeisterAn There we go - squashed into a single commit. Thank you for the collaboration so far 🚀

@HofmeisterAn
Copy link
Collaborator

Closes #481.

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.

Support setting ulimits on Docker containers
2 participants