[6.16.z] Replace library/busybox with lighter container repo - #20858
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 GuideUpdates container-related tests and configuration to use a lighter container image in place of library/busybox, and adjusts the test to run containers in detached mode while more safely handling container cleanup. Sequence diagram for updated container test lifecyclesequenceDiagram
actor Tester
participant TestCase as TestCase_container_management
participant DockerClient as Docker_client
Tester->>TestCase: run
TestCase->>DockerClient: pull jmalloc/echo-server
DockerClient-->>TestCase: image pulled
TestCase->>DockerClient: run container detached
DockerClient-->>TestCase: container_id
TestCase->>DockerClient: perform test operations
DockerClient-->>TestCase: responses
TestCase->>DockerClient: stop container
DockerClient-->>TestCase: stopped
TestCase->>DockerClient: remove container
DockerClient-->>TestCase: removed
TestCase-->>Tester: report test result
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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_idcan be referenced in thefinallyblock before assignment if the regex match fails; initialize it (e.g., toNone) before thetryand guard accordingly. - You introduced
re.matchintest_container_management.pybut there is no correspondingimport rein the diff; ensure the module is imported in this file. - After changing the default
container.upstream_nametojmalloc/echo-server, consider whethercontainer.alternative_upstream_namesshould also be updated to include or align with this new default.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_positive_pull_image`, `container_id` can be referenced in the `finally` block before assignment if the regex match fails; initialize it (e.g., to `None`) before the `try` and guard accordingly.
- You introduced `re.match` in `test_container_management.py` but there is no corresponding `import re` in the diff; ensure the module is imported in this file.
- After changing the default `container.upstream_name` to `jmalloc/echo-server`, consider whether `container.alternative_upstream_names` should also be updated to include or align with this new default.
## Individual Comments
### Comment 1
<location path="tests/foreman/cli/test_container_management.py" line_range="97-102" />
<code_context>
assert result.status == 0
try:
- result = module_container_contenthost.execute(f'docker run {repo["published-at"]}')
+ result = module_container_contenthost.execute(
+ 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:
</code_context>
<issue_to_address>
**issue:** Potential use of uninitialized `container_id` and missing assertion on container ID format in the test
`container_id` is only set inside the `if match:` block but used in `finally`, so if `re.match` returns `None` you’ll hit an `UnboundLocalError` there. Initialize `container_id = None` before the `try:` and have the cleanup check it safely. Also, instead of silently skipping cleanup when `match` is falsy, assert that the output is a valid container ID, e.g.:
```python
container_id = None
...
container_id = result.stdout.strip()
assert re.match(r"^[0-9a-f]+$", container_id), f"Unexpected container id: {container_id!r}"
```
so the test explicitly verifies the expected format and fails if Docker returns something unexpected.
</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
+97
to
+102
| result = module_container_contenthost.execute( | ||
| f'docker run -d {repo["published-at"]}' | ||
| ) | ||
| assert result.status == 0 | ||
| match = re.match(r'^[0-9a-f]+$', result.stdout) | ||
| if match: |
Contributor
There was a problem hiding this comment.
issue: Potential use of uninitialized container_id and missing assertion on container ID format in the test
container_id is only set inside the if match: block but used in finally, so if re.match returns None you’ll hit an UnboundLocalError there. Initialize container_id = None before the try: and have the cleanup check it safely. Also, instead of silently skipping cleanup when match is falsy, assert that the output is a valid container ID, e.g.:
container_id = None
...
container_id = result.stdout.strip()
assert re.match(r"^[0-9a-f]+$", container_id), f"Unexpected container id: {container_id!r}"so the test explicitly verifies the expected format and fails if Docker returns something unexpected.
Contributor
Author
|
Collaborator
|
PRT Result |
Satellite-QE
added
the
PRT-Failed
Contributor
Author
|
PRT failures are caused by the fact we dont use IPv6 for Satellite 6.16 So automation in 6.16.z branch is not IPv6 ready |
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
#20855
Summary by Sourcery
Update container test image configuration and improve container lifecycle handling in CLI tests.
Enhancements:
Tests:
Chores: