Skip to content

Commit eaea7ad

Browse files
authored
refactor: container client configures HostMount globally (#2506)
`HostMount` is already configured globally by the CLI (set for each command). Saving this configuration in the docker client eases the use of the container client as we don't need to keep passing docker configuration options through helper functions. Also refactors `docker.New()` to provide all optional arguments via a struct object rather than positional argument. This eases the implementation of updating images (and simplifies the existing command runners)
1 parent 181b7c3 commit eaea7ad

File tree

4 files changed

+48
-36
lines changed

4 files changed

+48
-36
lines changed

internal/docker/docker.go

Lines changed: 33 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -59,17 +59,17 @@ type Docker struct {
5959
// The group ID to run the container as.
6060
gid string
6161

62+
// HostMount specifies a mount point from the Docker host into the Docker
63+
// container. The format is "{host-dir}:{local-dir}".
64+
HostMount string
65+
6266
// run runs the docker command.
6367
run func(args ...string) error
6468
}
6569

6670
// BuildRequest contains all the information required for a language
6771
// container to run the build command.
6872
type BuildRequest struct {
69-
// HostMount specifies a mount point from the Docker host into the Docker
70-
// container. The format is "{host-dir}:{local-dir}".
71-
HostMount string
72-
7373
// LibraryID specifies the ID of the library to build.
7474
LibraryID string
7575

@@ -87,10 +87,6 @@ type ConfigureRequest struct {
8787
// ApiRoot specifies the root directory of the API specification repo.
8888
ApiRoot string
8989

90-
// HostMount specifies a mount point from the Docker host into the Docker
91-
// container. The format is "{host-dir}:{local-dir}".
92-
HostMount string
93-
9490
// libraryID specifies the ID of the library to configure.
9591
LibraryID string
9692

@@ -118,10 +114,6 @@ type GenerateRequest struct {
118114
// ApiRoot specifies the root directory of the API specification repo.
119115
ApiRoot string
120116

121-
// HostMount specifies a mount point from the Docker host into the Docker
122-
// container. The format is "{host-dir}:{local-dir}".
123-
HostMount string
124-
125117
// LibraryID specifies the ID of the library to generate.
126118
LibraryID string
127119

@@ -147,12 +139,6 @@ type ReleaseInitRequest struct {
147139
// create a pull request. This flag is ignored if Push is set to true.
148140
Commit bool
149141

150-
// HostMount is used to remap Docker mount paths when running in environments
151-
// where Docker containers are siblings (e.g., Kokoro).
152-
// It specifies a mount point from the Docker host into the Docker container.
153-
// The format is "{host-dir}:{local-dir}".
154-
HostMount string
155-
156142
// LibrarianConfig is a pointer to the [config.LibrarianConfig] struct, holding
157143
// global files configuration in a language repository.
158144
LibrarianConfig *config.LibrarianConfig
@@ -180,14 +166,33 @@ type ReleaseInitRequest struct {
180166
State *config.LibrarianState
181167
}
182168

169+
// DockerOptions contains optional configuration parameters for invoking
170+
// docker commands.
171+
type DockerOptions struct {
172+
// UserUID is the user ID of the current user. It is used to run Docker
173+
// containers with the same user, so that created files have the correct
174+
// ownership.
175+
UserUID string
176+
// UserGID is the group ID of the current user. It is used to run Docker
177+
// containers with the same user, so that created files have the correct
178+
// ownership.
179+
UserGID string
180+
// HostMount is used to remap Docker mount paths when running in environments
181+
// where Docker containers are siblings (e.g., Kokoro).
182+
// It specifies a mount point from the Docker host into the Docker container.
183+
// The format is "{host-dir}:{local-dir}".
184+
HostMount string
185+
}
186+
183187
// New constructs a Docker instance which will invoke the specified
184188
// Docker image as required to implement language-specific commands,
185189
// providing the container with required environment variables.
186-
func New(workRoot, image, uid, gid string) (*Docker, error) {
190+
func New(workRoot, image string, options *DockerOptions) (*Docker, error) {
187191
docker := &Docker{
188-
Image: image,
189-
uid: uid,
190-
gid: gid,
192+
Image: image,
193+
uid: options.UserUID,
194+
gid: options.UserGID,
195+
HostMount: options.HostMount,
191196
}
192197
docker.run = func(args ...string) error {
193198
return docker.runCommand("docker", args...)
@@ -227,7 +232,7 @@ func (c *Docker) Generate(ctx context.Context, request *GenerateRequest) error {
227232
fmt.Sprintf("%s:/output", request.Output),
228233
fmt.Sprintf("%s:/source:ro", request.ApiRoot), // readonly volume
229234
}
230-
return c.runDocker(ctx, request.HostMount, CommandGenerate, mounts, commandArgs)
235+
return c.runDocker(ctx, CommandGenerate, mounts, commandArgs)
231236
}
232237

233238
// Build builds the library with an ID of libraryID, as configured in
@@ -257,7 +262,7 @@ func (c *Docker) Build(ctx context.Context, request *BuildRequest) error {
257262
"--repo=/repo",
258263
}
259264

260-
return c.runDocker(ctx, request.HostMount, CommandBuild, mounts, commandArgs)
265+
return c.runDocker(ctx, CommandBuild, mounts, commandArgs)
261266
}
262267

263268
// Configure configures an API within a repository, either adding it to an
@@ -302,7 +307,7 @@ func (c *Docker) Configure(ctx context.Context, request *ConfigureRequest) (stri
302307
mounts = append(mounts, fmt.Sprintf("%s/%s:/repo/%s:ro", request.RepoDir, globalFile, globalFile))
303308
}
304309

305-
if err := c.runDocker(ctx, request.HostMount, CommandConfigure, mounts, commandArgs); err != nil {
310+
if err := c.runDocker(ctx, CommandConfigure, mounts, commandArgs); err != nil {
306311
return "", err
307312
}
308313

@@ -337,15 +342,15 @@ func (c *Docker) ReleaseInit(ctx context.Context, request *ReleaseInitRequest) e
337342
fmt.Sprintf("%s:/output", request.Output),
338343
}
339344

340-
if err := c.runDocker(ctx, request.HostMount, CommandReleaseInit, mounts, commandArgs); err != nil {
345+
if err := c.runDocker(ctx, CommandReleaseInit, mounts, commandArgs); err != nil {
341346
return err
342347
}
343348

344349
return nil
345350
}
346351

347-
func (c *Docker) runDocker(_ context.Context, hostMount string, command Command, mounts []string, commandArgs []string) (err error) {
348-
mounts = maybeRelocateMounts(hostMount, mounts)
352+
func (c *Docker) runDocker(_ context.Context, command Command, mounts []string, commandArgs []string) (err error) {
353+
mounts = maybeRelocateMounts(c.HostMount, mounts)
349354
args := []string{
350355
"run",
351356
"--rm", // Automatically delete the container after completion

internal/docker/docker_test.go

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,10 @@ func TestNew(t *testing.T) {
3535
testUID = "1000"
3636
testGID = "1001"
3737
)
38-
d, err := New(testWorkRoot, testImage, testUID, testGID)
38+
d, err := New(testWorkRoot, testImage, &DockerOptions{
39+
UserUID: testUID,
40+
UserGID: testGID,
41+
})
3942
if err != nil {
4043
t.Fatalf("New() error = %v", err)
4144
}
@@ -145,7 +148,8 @@ func TestDockerRun(t *testing.T) {
145148
{
146149
name: "Generate runs in docker",
147150
docker: &Docker{
148-
Image: testImage,
151+
Image: testImage,
152+
HostMount: "hostDir:localDir",
149153
},
150154
runCommand: func(ctx context.Context, d *Docker) error {
151155
generateRequest := &GenerateRequest{
@@ -154,7 +158,6 @@ func TestDockerRun(t *testing.T) {
154158
ApiRoot: testAPIRoot,
155159
Output: "hostDir",
156160
LibraryID: testLibraryID,
157-
HostMount: "hostDir:localDir",
158161
}
159162

160163
return d.Generate(ctx, generateRequest)
@@ -941,7 +944,10 @@ func TestReleaseInitRequestContent(t *testing.T) {
941944
},
942945
}
943946

944-
d, err := New(tmpDir, "test-image", "1000", "1000")
947+
d, err := New(tmpDir, "test-image", &DockerOptions{
948+
UserUID: "1000",
949+
UserGID: "1000",
950+
})
945951
if err != nil {
946952
t.Fatalf("New() failed: %v", err)
947953
}

internal/librarian/command.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,11 @@ func newCommandRunner(cfg *config.Config) (*commandRunner, error) {
138138
}
139139

140140
ghClient := github.NewClient(cfg.GitHubToken, gitHubRepo)
141-
container, err := docker.New(cfg.WorkRoot, image, cfg.UserUID, cfg.UserGID)
141+
container, err := docker.New(cfg.WorkRoot, image, &docker.DockerOptions{
142+
UserUID: cfg.UserUID,
143+
UserGID: cfg.UserGID,
144+
HostMount: cfg.HostMount,
145+
})
142146
if err != nil {
143147
return nil, err
144148
}

internal/librarian/generate.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,6 @@ func (r *generateRunner) runGenerateCommand(ctx context.Context, libraryID, outp
255255

256256
generateRequest := &docker.GenerateRequest{
257257
ApiRoot: apiRoot,
258-
HostMount: r.hostMount,
259258
LibraryID: libraryID,
260259
Output: outputDir,
261260
RepoDir: r.repo.GetDir(),
@@ -296,7 +295,6 @@ func (r *generateRunner) runBuildCommand(ctx context.Context, libraryID string)
296295
}
297296

298297
buildRequest := &docker.BuildRequest{
299-
HostMount: r.hostMount,
300298
LibraryID: libraryID,
301299
RepoDir: r.repo.GetDir(),
302300
State: r.state,
@@ -372,7 +370,6 @@ func (r *generateRunner) runConfigureCommand(ctx context.Context, outputDir stri
372370

373371
configureRequest := &docker.ConfigureRequest{
374372
ApiRoot: apiRoot,
375-
HostMount: r.hostMount,
376373
LibraryID: r.library,
377374
Output: outputDir,
378375
RepoDir: r.repo.GetDir(),

0 commit comments

Comments
 (0)