GenericContainerBuilder false pull policy should not pull image

[schummar]

Small issue I found by accident. Instead of this.pullPolicy the check should be this.pullPolicy.shouldPull().
The cleanOpts part makes sure that configs with undefined value are not included in the request - it caused issues for me.

[netlify]

✅ Deploy Preview for testcontainers-node ready!

Name	Link
🔨 Latest commit	9706aaa
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-node/deploys/666d740626692a00083674ea
😎 Deploy Preview	https://deploy-preview-771--testcontainers-node.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[cristianrgreco]

Thanks for raising @schummar. Could you give an example where providing an undefined value caused issues? For now I'd accept the change to generic-container-builder.ts which seems like a bug

[schummar]

Hmm, I am pretty sure I saw some kind of error when the parameter was passed as undefined. But I cannot reproduce it now. Maybe I mixed something up. I have reverted that change now.

[schummar]

Ok I guess that was the reason - it results in an error when running the build via Podman.

So I guess I bring the change back? I could also extract the logic to a util function to keep the code in docker-image-client.ts cleaner?

[cristianrgreco]

It is interesting that undefined is interpreted as an empty string and fails validation with Podman.

What about if we use null instead of undefined? If not I'd rather explicitly add the pull field to the opts if shouldPull returns true. I think that code is easier to reason about than something which dynamically alters the options.

Thanks again for raising this PR!

[schummar]

That's how the querystring lib handles it. Both undefined and null result in an empty string being sent. See https://runkit.com/embed/8n76jv2sc2lc

I changed it like you suggested. And I added a test case that fails without the fix.

[cristianrgreco]

Thanks @schummar, the implementation LGTM.

I think the test added is a duplicate of this one, which is unfortunately disabled for Podman

testcontainers-node/packages/testcontainers/src/generic-container/generic-container-dockerfile.test.ts

Lines 59 to 73 in 824cd8d

	// https://github.com/containers/podman/issues/17779
	if (!process.env.CI_PODMAN) {
	it("should use pull policy", async () => {
	const dockerfile = path.resolve(fixtures, "docker");
	const containerSpec = GenericContainer.fromDockerfile(dockerfile).withPullPolicy(PullPolicy.alwaysPull());
	await containerSpec.build();
	const dockerEventStream = await getDockerEventStream();
	const dockerPullEventPromise = waitForDockerEvent(dockerEventStream, "pull");
	await containerSpec.build();
	await dockerPullEventPromise;
	dockerEventStream.destroy();
	});
	}

[schummar]

That one test that the image is pulled when the pull policy says so. The new test tests that it doesn't pull when no pull policy is supplied.

[jonesbusy]

Without any other changes, upgrading from 10.9 to 10.10 break my build on my environment

[05:23:12.330+02:00] -     Could not find a working container runtime strategy
[05:23:12.330+02:00] - 
[05:23:12.330+02:00] -       18 |
[05:23:12.330+02:00] -       19 | export async function buildWireMockContainer(): Promise<StartedTestContainer> {
[05:23:12.330+02:00] -     > 20 |   return await new GenericContainer(`wiremock/wiremock:${WIREMOCK_VERSION}`)

Is suspect this PR cause my issue beucase it cause only GenericContainer to fail with Could not find a working container runtime strategy

I'm running testcontainer using kubedock (https://github.com/joyrex2001/kubedock)

[cristianrgreco]

Hi @jonesbusy, the code change in this PR only takes effect after a working container runtime is established. If you want help please raise an issue and fill out the template, providing logs, etc.

[jonesbusy]

Hi @jonesbusy, the code change in this PR only takes effect after a working container runtime is established. If you want help please raise an issue and fill out the template, providing logs, etc.

Hi, sure I will provide all the details like I did for #747

It's just that I didn't saw any other difference between 10.9 and 10.10 that might have cause the new issue I'm facing.

Thanks