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 SHM size to the container request #461

Merged
merged 2 commits into from
Jun 16, 2022

Conversation

EndPositive
Copy link
Contributor

@EndPositive EndPositive commented Jun 14, 2022

The code-way of specifying --shm-size on the Docker command-line.

E.g.

var shmsize = 1073741824 // = 1GB

I've been using this to start Selenium nodes, for example.

@codecov
Copy link

codecov bot commented Jun 14, 2022

Codecov Report

Merging #461 (4f15dd0) into main (d01c78a) will increase coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #461      +/-   ##
==========================================
+ Coverage   65.91%   66.11%   +0.19%     
==========================================
  Files          19       19              
  Lines        1200     1201       +1     
==========================================
+ Hits          791      794       +3     
+ Misses        303      302       -1     
+ Partials      106      105       -1     
Impacted Files Coverage Δ
container.go 87.23% <ø> (ø)
docker.go 66.90% <100.00%> (+0.42%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d01c78a...4f15dd0. Read the comment docs.

Copy link
Member

@mdelapenya mdelapenya 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 taking the time to work on this contribution, it's much than appreciated!

My comments: although the documentation for shm uses this format:

Size of /dev/shm. The format is . number must be greater than 0. Unit is optional and can be b (bytes), k (kilobytes), m (megabytes), or g (gigabytes). If you omit the unit, the system uses bytes. If you omit the size entirely, the system uses 64m.

which could advocate for a more elaborated format, I think using a format with just the bytes is great for simplicity.

What I'd take a look is at the consistency with both Docker client and the Java project, which uses bytes as default unit.

Once updated to use bytes, I think this PR is perfect!

@EndPositive
Copy link
Contributor Author

Hey 👋, thanks for the detailed comment!

I agree to keep it simple and not bother with parsing of the different units.

Good point about the default unit though. It looks like I made a mistake writing that docstring. By default, Golang's HostConfig is in Bytes, and since I directly pass our container request to that hostconfig, it should have also been in bytes.

I've updated the docstring ^^

@mdelapenya mdelapenya merged commit fa714e3 into testcontainers:main Jun 16, 2022
@EndPositive EndPositive deleted the feature/shm-size branch June 16, 2022 15:33
@mdelapenya mdelapenya added the feature New functionality or new behaviors on the existing one label Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or new behaviors on the existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants