[6.17.z] Replace library/busybox with lighter container repo - #20857
Merged
lpramuk merged 1 commit intoFeb 25, 2026
Conversation
(cherry picked from commit 8ac73bc)
lpramuk
added
No-CherryPick
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR replaces the default container test image repository from the large Sequence diagram for updated container CLI test run and cleanupsequenceDiagram
actor Tester
participant RobotteloTest as Robottelo_test
participant ForemanCLI as Foreman_CLI
participant ContainerRuntime as Container_runtime
participant ContainerRegistry as Container_registry
Tester->>RobotteloTest: start_container_cli_test
RobotteloTest->>ForemanCLI: hammer_container_run image=jmalloc/echo-server detached=true
ForemanCLI->>ContainerRuntime: run_container image=jmalloc/echo-server detached=true
ContainerRuntime->>ContainerRegistry: pull_image jmalloc/echo-server
ContainerRegistry-->>ContainerRuntime: image_layers
ContainerRuntime-->>ForemanCLI: container_id
ForemanCLI-->>RobotteloTest: container_id
Note over RobotteloTest,ForemanCLI: Test logic uses returned container_id for cleanup
RobotteloTest->>ForemanCLI: hammer_container_stop container_id
ForemanCLI->>ContainerRuntime: stop_container container_id
ContainerRuntime-->>ForemanCLI: stopped
ForemanCLI-->>RobotteloTest: stopped
RobotteloTest->>ForemanCLI: hammer_container_remove container_id
ForemanCLI->>ContainerRuntime: remove_container container_id
ContainerRuntime-->>ForemanCLI: removed
ForemanCLI-->>RobotteloTest: removed
RobotteloTest-->>Tester: test_passed
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
lpramuk
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
test_positive_pull_image,container_idis only conditionally assigned inside thetryblock but referenced in thefinally, which can raise anUnboundLocalError; initializecontainer_id = Nonebefore thetryand then check it in thefinally. - When extracting the container ID from
docker run -doutput, consider stripping or reading only the first line fromresult.stdoutbefore applying the regex to avoid issues with trailing newlines or additional output.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_positive_pull_image`, `container_id` is only conditionally assigned inside the `try` block but referenced in the `finally`, which can raise an `UnboundLocalError`; initialize `container_id = None` before the `try` and then check it in the `finally`.
- When extracting the container ID from `docker run -d` output, consider stripping or reading only the first line from `result.stdout` before applying the regex to avoid issues with trailing newlines or additional output.
## Individual Comments
### Comment 1
<location path="tests/foreman/cli/test_container_management.py" line_range="101-103" />
<code_context>
+ f'docker run -d {repo["published-at"]}'
+ )
assert result.status == 0
+ match = re.match(r'^[0-9a-f]+$', result.stdout)
+ if match:
+ container_id = match.group(0)
finally:
# Stop and remove the container
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential use of `container_id` before assignment and missing negative-path handling when the regex does not match
`container_id` is only set inside the `if match:` block but is always referenced in `finally`. If `re.match` returns `None` (e.g., due to trailing whitespace or extra output), this will raise `UnboundLocalError`, and even if pre-initialized, cleanup would be skipped when there’s no match.
Consider initializing `container_id = None` before the `try`, and then either normalizing the output (e.g., strip / split to get the ID) or failing the test explicitly when `match` is `None` instead of silently skipping cleanup.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+101
to
+103
| match = re.match(r'^[0-9a-f]+$', result.stdout) | ||
| if match: | ||
| container_id = match.group(0) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Potential use of container_id before assignment and missing negative-path handling when the regex does not match
container_id is only set inside the if match: block but is always referenced in finally. If re.match returns None (e.g., due to trailing whitespace or extra output), this will raise UnboundLocalError, and even if pre-initialized, cleanup would be skipped when there’s no match.
Consider initializing container_id = None before the try, and then either normalizing the output (e.g., strip / split to get the ID) or failing the test explicitly when match is None instead of silently skipping cleanup.
Contributor
Author
|
Collaborator
|
PRT Result |
Satellite-QE
added
the
PRT-Passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Manual cherrypick of PR: #20671
(cherry picked from commit 8ac73bc)
Problem Statement
Gradually over time the
library/busyboxrepo is becoming more and more huge.The problem with repo sync arises especially in IPv6 environment where the traffic goes over IPv6-to-4 proxy.
Testing revealed that syncing over IPv4 takes approx. 10 min while syncing over IPv6 takes 25 min and tends to time out quite often.
Solution
Find more suitable replacement for testing container repo target
Related Issues
SAT-41831
#20854
Summary by Sourcery
Replace the default test container image with a lighter upstream repository and adjust container management tests accordingly.
Enhancements:
Tests: