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 setting user, workingDir and env when executing a command in a container #668

Merged
merged 2 commits into from
Nov 23, 2023
Merged

Conversation

LNSD
Copy link
Contributor

@LNSD LNSD commented Nov 10, 2023

This pull request introduces support for specifying User, WorkingDir, and Env options during container execution in both Docker (API docs) and Podman (API docs), enhancing customization.

Changes:

Added support to StartedTestContainer.exec(command, opts) function to provide the following options:

  1. user:

    • Users can set the user, and optionally, group to run the exec process inside the container. Format is one of: user, user:group, uid, or uid:gid.
  2. workingDir:

    • The WorkingDir option allows users to define the working directory for the exec process inside the container.
  3. env:

    • Users can now include the Env option when executing commands, allowing for environment variable customization.

Example:

const container = await new GenericContainer("alpine")
  .withCommand(["sleep", "infinity"])
  .start();

const { output, exitCode } = await container.exec(["echo", "hello", "world"], {
	workingDir: "/app/src/",
	user: "alpine",
	env: {
		"VAR1": "enabled",
		"VAR2": "full",
	}
});

Your feedback and testing contributions are appreciated.

Copy link

netlify bot commented Nov 10, 2023

Deploy Preview for testcontainers-node ready!

Name Link
🔨 Latest commit e794806
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-node/deploys/655f60910eafcb0009fa44e7
😎 Deploy Preview https://deploy-preview-668--testcontainers-node.netlify.app/features/containers
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@javierlopezdeancos javierlopezdeancos left a comment

Choose a reason for hiding this comment

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

I think the addition of these options is very useful and necessary when executing commands within a container. 💯

Just some nits relevant to the format that should give us prettier.

@javierlopezdeancos
Copy link
Contributor

javierlopezdeancos commented Nov 11, 2023

What difference and advantage we have doing

const container = await new GenericContainer("alpine")
  .withCommand(["sleep", "infinity"])
  .start();

const { output, exitCode } = await container.exec(["echo", "hello", "world"], {
	workingDir: "/app/src/",
	user: "alpine",
	env: {
		"VAR1": "enabled",
		"VAR2": "full",
	}
});

instead or doing 🤔 ?

const startedContainer = await new GenericContainer("alpine")
  .withWorkingDir("/app/src/")
  .withUser("alpine")
  .withEnvironment({
        "VAR1": "enabled",
         "VAR2": "full",
   })
  .withCommand(["sleep", "infinity"])
  .start();

const { output, exitCode } = await startedContainer.exec(["echo", "hello", "world"]);

apart from being able to redefine these options in each execution of a command?

@LNSD
Copy link
Contributor Author

LNSD commented Nov 11, 2023

The two things are different.

What difference and advantage we have doing

const container = await new GenericContainer("alpine")
  .withCommand(["sleep", "infinity"])
  .start();

const { output, exitCode } = await container.exec(["echo", "hello", "world"], {
	workingDir: "/app/src/",
	user: "alpine",
	env: {
		"VAR1": "enabled",
		"VAR2": "full",
	}
});

The first, which I added support for, specifies the current working directory for the specific command you are running in an already-running container. So, you are only affecting the command you execute. You must specify the workingDir option for every new command you want to execute in the container.

The use case here is:

I want to run a command in a directory different than the container running the main process.

For example, I want to modify on-the-fly a configuration file stored in /etc/app/app.conf, by executing a sed command, while the container's main process is running and serving requests.

instead or doing 🤔 ?

const startedContainer = await new GenericContainer("alpine")
  .withWorkingDir("/app/src/")
  .withUser("alpine")
  .withEnvironment({
        "VAR1": "enabled",
         "VAR2": "full",
   })
  .withCommand(["sleep", "infinity"])
  .start();

const { output, exitCode } = await startedContainer.exec(["echo", "hello", "world"]);

apart from being able to redefine these options in each execution of a command?

This second case affects the current working directory of the new container you will create after calling start().

This ENV/WORKDIR/CMD override is a "global" modification that will affect all commands running within the container (the ENTRYPOINT+CMD and all the processes executed using the exec() function). By using withCommand(...), you are also changing the new container's start command.

For reference:

@javierlopezdeancos
Copy link
Contributor

The feature has really sense to me @LNSD thanks to clarify with all this great explanation

Copy link
Contributor

@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.

Hi @LNSD, first thank you so much for opening the PR and for providing such a valid use case. We love contributions!!

Second, I'm core maintainer of Testcontainer Go, stepping ahead because @cristianrgreco is on PTO, and seeing this PR with a lot of value. So please take my review comments as a non-typescript developer at all 🙏 As you'll read, my comments are mostly related to style and consistency with existing patterns, not related at all with the "business" of the code, which initially LGTM.

@mdelapenya
Copy link
Contributor

BTW it'd be important if you could add tests demonstrating the new behaviour 🙏

@LNSD
Copy link
Contributor Author

LNSD commented Nov 13, 2023

Added an integration test 🧪 covering the new functionality: faa32d7

Copy link
Contributor

@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.

This LGTM, added a comment regarding the added test, but other than that, thank you for this contribution. Looks a great improvement!

BTW I'll not merge it immediately, as I'd like to give @cristianrgreco a couple of days to have the chance to take a look if needed.

@LNSD
Copy link
Contributor Author

LNSD commented Nov 15, 2023

Rebased the PR on top of the CI fix 🔁

@cristianrgreco
Copy link
Collaborator

cristianrgreco commented Nov 23, 2023

Thanks for raising the PR @LNSD, very well tested and documented 🙂

Because ExecOptions is now part of the public API:

exec(command: string | string[], opts?: Partial<ExecOptions>): Promise<ExecResult>;
We'll need to export the type here:
export { InspectResult, Content, ExecResult } from "./types";

So that a user could do for example:

import { ExecOptions, ExecResult } from "testcontainers";

const options: ExecOptions = { ... };
const result = await container.exec(..., options); 

Other than that it LGTM

@@ -19,7 +19,7 @@ export { Network, StartedNetwork, StoppedNetwork } from "./network/network";
export { Wait } from "./wait-strategies/wait";
export { StartupCheckStrategy, StartupStatus } from "./wait-strategies/startup-check-strategy";
export { PullPolicy, ImagePullPolicy } from "./utils/pull-policy";
export { InspectResult, Content, ExecResult } from "./types";
export { InspectResult, Content, ExecOptions, ExecResult } from "./types";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ✔️ 😁

@cristianrgreco cristianrgreco added enhancement New feature or request minor Backward compatible functionality labels Nov 23, 2023
@cristianrgreco cristianrgreco changed the title Add support for container exec options Add support for setting user, workingDir and env when executing a command in a container Nov 23, 2023
@cristianrgreco cristianrgreco merged commit 6a54b47 into testcontainers:main Nov 23, 2023
75 checks passed
@LNSD LNSD deleted the container-exec-options branch November 23, 2023 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor Backward compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants