Skip to content

Fix image-pull-proxy integration test#88

Merged
chmeliik merged 2 commits intokonflux-ci:mainfrom
chmeliik:fix-build-test
Mar 25, 2026
Merged

Fix image-pull-proxy integration test#88
chmeliik merged 2 commits intokonflux-ci:mainfrom
chmeliik:fix-build-test

Conversation

@chmeliik
Copy link
Copy Markdown
Contributor

Previously, the test only worked if it was the first test that uses the baseImage. Otherwise, the image was already stored in the local container storage and no additional requests went through the proxy.

Fix by using a clean container storage dir for this test.

Previously, the framework made it possible to mount multiple host
directories to the same container directory, but not vice versa.

That is the opposite of what container engines allow. It is possible to
mount the same host dir to multiple container dirs, but not the other
way around.

Change the behavior to match container engines. Calling AddVolume or
AddVolumeWithOptions twice with the same containerDir will now mount the
last hostDir, and calling it twice with the same hostDir will mount it
at both the containerDirs.

Assisted-by: Claude
Signed-off-by: Adam Cmiel <acmiel@redhat.com>
Previously, the test only worked if it was the first test that uses the
baseImage. Otherwise, the image was already stored in the local
container storage and no additional requests went through the proxy.

Fix by using a clean container storage dir for this test.

Signed-off-by: Adam Cmiel <acmiel@redhat.com>
@chmeliik chmeliik requested a review from a team as a code owner March 25, 2026 14:37
@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Mar 25, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📐 Spec deviations (0)

Grey Divider


Remediation recommended

1. Cleanup before error check 🐞 Bug ⛯ Reliability
Description
The ImagePullProxy test registers t.Cleanup() using containerStorage before asserting
createContainerStorageDir() succeeded, so if createContainerStorageDir returns an error with a
non-empty path the test will still run container-based cleanup during a failing setup step.
Code

integration_tests/build_test.go[R2468-2470]

+		containerStorage, err := createContainerStorageDir()
+		t.Cleanup(func() { removeContainerStorageDir(containerStorage) })
+		Expect(err).ToNot(HaveOccurred())
Evidence
createContainerStorageDir() can return a non-empty tmpDir together with an error (e.g., if os.Chmod
fails it returns (tmpDir, err)). In that case, the newly-added cleanup will still execute and
removeContainerStorageDir() will run cleanupContainerStorageDir(), which starts a build container to
delete files. This is unnecessary for a setup failure and can slow down/obscure the primary failure.

integration_tests/build_test.go[2464-2471]
integration_tests/build_test.go[102-129]
integration_tests/build_test.go[144-192]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`TestBuild/ImagePullProxy` registers cleanup for `containerStorage` before checking `err` from `createContainerStorageDir()`. If `createContainerStorageDir()` fails after creating the directory (e.g., chmod failure), cleanup will still run and may start an extra container during a failing setup path.

### Issue Context
- `createContainerStorageDir()` can return `(tmpDir, err)` when chmod fails.
- `removeContainerStorageDir()` calls `cleanupContainerStorageDir()` which starts a container.

### Fix Focus Areas
- Ensure cleanup still happens when a partial directory was created, but avoid heavy container-based cleanup on the error path.
- Register `t.Cleanup` only when `err == nil`, and on `err != nil` do a best-effort direct removal (e.g., `os.RemoveAll(containerStorage)`), then fail the test.

- integration_tests/build_test.go[2464-2471]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@snyk-io
Copy link
Copy Markdown

snyk-io bot commented Mar 25, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@chmeliik chmeliik merged commit 320f8b2 into konflux-ci:main Mar 25, 2026
7 checks passed
@chmeliik chmeliik deleted the fix-build-test branch March 25, 2026 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants