Allow uid:gid to be specified during container creation #363
Conversation
| @@ -675,6 +675,7 @@ func (p *DockerProvider) CreateContainer(ctx context.Context, req ContainerReque | |||
| Labels: req.Labels, | |||
| Cmd: req.Cmd, | |||
| Hostname: req.Hostname, | |||
| User: req.User, | |||
There was a problem hiding this comment.
What would happen if the req.User is empty? Will the docker client understand that as root?
There was a problem hiding this comment.
User is a string field, so it appears that an empty string equates to not setting a UID at all:
Confirmed with these tests:
package main
import (
"bytes"
"context"
"fmt"
"github.com/testcontainers/testcontainers-go"
"github.com/testcontainers/testcontainers-go/wait"
)
func main() {
tests := []testcontainers.ContainerRequest{
{
Image: "debian:latest",
Cmd: []string{"sh", "-c", "id -u"},
WaitingFor: wait.ForExit(),
},
{
Image: "debian:latest",
User: "60125",
Cmd: []string{"sh", "-c", "id -u"},
WaitingFor: wait.ForExit(),
},
}
for _, test := range tests {
ctx := context.Background()
c, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{
ContainerRequest: test,
Started: true,
})
if err != nil {
panic(err)
}
buf := new(bytes.Buffer)
r, _ := c.Logs(ctx)
buf.ReadFrom(r)
fmt.Printf("User: expected: %s; got: %s\n", test.User, buf.String())
}
}Output:
2021/10/05 08:17:34 Starting container id: fa62829446d6 image: quay.io/testcontainers/ryuk:0.2.3
2021/10/05 08:17:35 Waiting for container id fa62829446d6 image: quay.io/testcontainers/ryuk:0.2.3
2021/10/05 08:17:35 Container is ready id: fa62829446d6 image: quay.io/testcontainers/ryuk:0.2.3
2021/10/05 08:17:35 Starting container id: 1b75176f40da image: debian:latest
2021/10/05 08:17:35 Waiting for container id 1b75176f40da image: debian:latest
2021/10/05 08:17:35 Container is ready id: 1b75176f40da image: debian:latest
User: expected: ; got: 0
2021/10/05 08:17:35 Starting container id: 063145d38a97 image: debian:latest
2021/10/05 08:17:36 Waiting for container id 063145d38a97 image: debian:latest
2021/10/05 08:17:36 Container is ready id: 063145d38a97 image: debian:latest
User: expected: 60125; got: 60125
There was a problem hiding this comment.
Would you mind adding two tests in your PR: one for adding the User, and another one without it?
Besides that, we could only set the User field if req.User is not empty
There was a problem hiding this comment.
@mdelapenya sure thing! Tests have been added.
Codecov Report
@@ Coverage Diff @@
## master #363 +/- ##
==========================================
+ Coverage 61.91% 62.64% +0.72%
==========================================
Files 15 15
Lines 1011 1012 +1
==========================================
+ Hits 626 634 +8
+ Misses 288 279 -9
- Partials 97 99 +2
Continue to review full report at Codecov.
|
| if err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
Nitpick: these if-statements can be replaced by calls to require.NoError(t, err) if you import github.com/stretchr/testify/require
There was a problem hiding this comment.
True. I used t.Fatal(err) here to match other tests for this package.
There was a problem hiding this comment.
If you agree, would you mind sending the changes related to the require.NoError, or assert.Nil, in a follow up PR?
|
@mdelapenya Is there anything else needed for this PR? Could the pending workflow be run? |
|
@gianarb this is ready to go. Merging? |
|
Thanks a lot! |
Fixes #361
Adds a
Userfield to ContainerRequest and includes that value in the container config so thatuid[:gid]can be specified for the container.