Compare nginx version against the major OS version#375
Merged
phracek merged 3 commits intosclorg:masterfrom Apr 21, 2026
Merged
Compare nginx version against the major OS version#375phracek merged 3 commits intosclorg:masterfrom
phracek merged 3 commits intosclorg:masterfrom
Conversation
In case image is less then previous major OS version, then the test passed. In case image is higher then previous major OS version, then the test is skipped. Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
📝 WalkthroughWalkthroughAdds versioned test stubs for container-size tests, removes several 1.24 test wrapper files, reformats and documents existing tests, and introduces a new test module that compares built Nginx image sizes against published reference images. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as "test_compare_container_sizes()"
participant Lib as "ContainerTestLib"
participant Utils as "get_previous_os_version()"
participant ImageUtil as "get_public_image_name()"
participant Compare as "ContainerCompareClass"
Test->>Lib: setup_method(VARS.IMAGE_NAME, s2i_image=True)
Test->>Utils: get_previous_os_version(VARS.OS)
Utils-->>Test: previous_version
Test->>ImageUtil: get_public_image_name(previous_version, base_image_name="nginx", version=VARS.VERSION, stage_registry=True)
ImageUtil-->>Test: published_image_name
Test->>Compare: is_uncompressed_image_smaller(VARS.IMAGE_NAME, published_image_name)
Compare-->>Test: uncompressed_result
Test->>Compare: is_compressed_image_smaller(VARS.IMAGE_NAME, published_image_name)
Compare-->>Test: compressed_result
Test-->>Test: decide skip/assert based on results
Test->>Lib: teardown_method() / cleanup()
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Pull Request validationFailed🔴 Failed or pending statuses:
🔴 Review - Missing review from a member (1 required) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/test_container_sizes.py`:
- Around line 32-37: The test calls get_public_image_name with
base_image_name="postgresql", causing it to compare against the wrong published
image; change the argument in that call to base_image_name="nginx" (the repo
BASE_IMAGE_NAME pattern) so the published_image_name is built for nginx; locate
the call to get_public_image_name in test_container_sizes.py (the invocation
using get_previous_os_version(VARS.OS) and VARS.VERSION) and replace the
base_image_name value accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e707e77-cc58-4a67-b6aa-43d7030ff950
📒 Files selected for processing (10)
1.20/test/test_container_sizes.py1.22/test/test_container_sizes.py1.24/test/test-lib-nginx.sh1.24/test/test-lib-openshift.sh1.24/test/test-lib-remote-openshift.sh1.24/test/test-openshift.yaml1.24/test/test_container_sizes.py1.26/test/test_container_sizes.pytest/test_container_basics.pytest/test_container_sizes.py
💤 Files with no reviewable changes (4)
- 1.24/test/test-lib-remote-openshift.sh
- 1.24/test/test-lib-openshift.sh
- 1.24/test/test-openshift.yaml
- 1.24/test/test-lib-nginx.sh
Get proper base_image_name. Nginx is correct ona Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
Member
Author
|
[test-pytest] |
Testing Farm results
|
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
Member
Author
|
[test-pytest] |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/test_container_sizes.py`:
- Around line 41-53: Rename the misspelled local variable is_less_uncopressed to
is_less_uncompressed everywhere it's used in test/test_container_sizes.py:
update the assignment from
ContainerCompareClass.is_uncompressed_image_smaller(...) to store into
is_less_uncompressed, update the conditional that checks it (if not
is_less_uncompressed or not is_less_compressed) and update the pytest.skip
message interpolation to reference is_less_uncompressed instead of
is_less_uncopressed so the variable name matches the intended meaning and avoids
the typo in CI logs.
- Around line 27-53: The test should catch lookup/network failures and log
comparison results: wrap the call to get_public_image_name(...
get_previous_os_version(VARS.OS) ...) in a try/except that calls pytest.skip
when get_previous_os_version or get_public_image_name raises (include a message
with the exception), and similarly wrap the calls to
ContainerCompareClass.is_uncompressed_image_smaller and
is_compressed_image_smaller to catch exceptions and skip with an explanatory
message; after obtaining their return values, log the raw is_less_uncompressed
and is_less_compressed at info level and explicitly treat None or other falsy
non-boolean returns as a transient error (skip with a message indicating
possible network/registry issue) rather than a plain size failure before calling
pytest.skip for genuine size comparisons.
- Around line 41-48: The call to
ContainerCompareClass.is_compressed_image_smaller uses the wrong keyword
argument name; change the argument from published_image_name to published_image
so it matches the method signature (same parameter used in
ContainerCompareClass.is_uncompressed_image_smaller); update the call site in
the test to pass published_image=published_image_name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d00928f4-aa15-47e1-a6c7-5100bdfade50
📒 Files selected for processing (2)
test/conftest.pytest/test_container_sizes.py
Comment on lines
+27
to
+53
| def test_compare_container_sizes(self): | ||
| """ | ||
| Test the size of the Nginx container against the | ||
| already published container images. | ||
| """ | ||
| skip_if_version_not_minimal() | ||
| if not VARS.OS.startswith("rhel"): | ||
| pytest.skip("Skipping container size comparison for non-RHEL OS.") | ||
| published_image_name = get_public_image_name( | ||
| os_name=get_previous_os_version(VARS.OS), | ||
| base_image_name="nginx", | ||
| version=VARS.VERSION, | ||
| stage_registry=True, | ||
| ) | ||
| is_less_uncopressed = ContainerCompareClass.is_uncompressed_image_smaller( | ||
| built_image_name=VARS.IMAGE_NAME, | ||
| published_image=published_image_name, | ||
| ) | ||
| is_less_compressed = ContainerCompareClass.is_compressed_image_smaller( | ||
| built_image_name=VARS.IMAGE_NAME, | ||
| published_image_name=published_image_name, | ||
| ) | ||
| if not is_less_uncopressed or not is_less_compressed: | ||
| pytest.skip( | ||
| f"Container size is not less than the published image {published_image_name}. " | ||
| f"Uncompressed image size: {is_less_uncopressed}, Compressed image size: {is_less_compressed}" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm get_previous_os_version behavior for the lowest supported RHEL and return type of the comparison helpers.
python - <<'PY'
import inspect
try:
from container_ci_suite.utils import get_previous_os_version, get_public_image_name
print("get_previous_os_version:", inspect.signature(get_previous_os_version))
for os_name in ("rhel8", "rhel9", "rhel10"):
try:
print(os_name, "->", get_previous_os_version(os_name))
except Exception as e:
print(os_name, "raises", type(e).__name__, e)
except Exception as e:
print("import failed:", e)
PYRepository: sclorg/nginx-container
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Explore repository structure to find test file and helper function implementations
git ls-files | head -20
echo "---"
fd -t f "test_container_sizes"
fd -t f "\.py$" | grep -E "(utils|container)" | head -20Repository: sclorg/nginx-container
Length of output: 780
🏁 Script executed:
#!/bin/bash
# Read the test file mentioned in the review
head -100 test/test_container_sizes.pyRepository: sclorg/nginx-container
Length of output: 1925
🏁 Script executed:
#!/bin/bash
# Search for the helper functions referenced in the test
rg "def get_previous_os_version|def get_public_image_name|class ContainerCompareClass" -A 10Repository: sclorg/nginx-container
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check conftest.py and look for dependency definitions
cat -n test/conftest.pyRepository: sclorg/nginx-container
Length of output: 1763
🏁 Script executed:
#!/bin/bash
# Look for dependency/requirements files
fd -t f "requirements|setup|pyproject" | xargs ls -la 2>/dev/null
git ls-files | grep -E "requirements|setup|pyproject"Repository: sclorg/nginx-container
Length of output: 1281
🏁 Script executed:
#!/bin/bash
# Check .gitmodules to see if container_ci_suite is a submodule
cat .gitmodules 2>/dev/null || echo "No .gitmodules found"Repository: sclorg/nginx-container
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Search for any existing try/except patterns in test files or error handling
rg "try:|except|raise" test/ --type py | head -30Repository: sclorg/nginx-container
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Check GitHub workflows to understand test execution
cat -n .github/workflows/container-tests.yml | head -100Repository: sclorg/nginx-container
Length of output: 684
🏁 Script executed:
#!/bin/bash
# Search for all uses of get_previous_os_version in the test directory
rg "get_previous_os_version" test/Repository: sclorg/nginx-container
Length of output: 261
🏁 Script executed:
#!/bin/bash
# Check for any documentation about test dependencies
cat README.md | grep -A 5 -B 5 "test\|pytest\|container_ci_suite" | head -40Repository: sclorg/nginx-container
Length of output: 1310
🏁 Script executed:
#!/bin/bash
# Check if there's any mention of how to handle edge cases
rg "rhel8|rhel9|rhel10" test/ -B 2 -A 2Repository: sclorg/nginx-container
Length of output: 1132
Wrap helper calls in try/except and add logging for comparison results.
The test will error (not skip) if get_previous_os_version raises when called with an unsupported RHEL version. Since the code only defines tags for rhel8, rhel9, and rhel10 in conftest.py, calling get_previous_os_version("rhel8") could fail if there's no defined previous version. Wrap the get_public_image_name call (and potentially the comparison methods) in a try/except block that skips the test on lookup failure.
Additionally, add logging of the raw is_less_uncompressed and is_less_compressed values at info level. If these helpers silently return None/falsy on network errors (e.g., registry unreachable), the current skip message won't distinguish a true size comparison failure from a transient network issue, making CI flakiness harder to diagnose.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/test_container_sizes.py` around lines 27 - 53, The test should catch
lookup/network failures and log comparison results: wrap the call to
get_public_image_name(... get_previous_os_version(VARS.OS) ...) in a try/except
that calls pytest.skip when get_previous_os_version or get_public_image_name
raises (include a message with the exception), and similarly wrap the calls to
ContainerCompareClass.is_uncompressed_image_smaller and
is_compressed_image_smaller to catch exceptions and skip with an explanatory
message; after obtaining their return values, log the raw is_less_uncompressed
and is_less_compressed at info level and explicitly treat None or other falsy
non-boolean returns as a transient error (skip with a message indicating
possible network/registry issue) rather than a plain size failure before calling
pytest.skip for genuine size comparisons.
Comment on lines
+41
to
+53
| is_less_uncopressed = ContainerCompareClass.is_uncompressed_image_smaller( | ||
| built_image_name=VARS.IMAGE_NAME, | ||
| published_image=published_image_name, | ||
| ) | ||
| is_less_compressed = ContainerCompareClass.is_compressed_image_smaller( | ||
| built_image_name=VARS.IMAGE_NAME, | ||
| published_image_name=published_image_name, | ||
| ) | ||
| if not is_less_uncopressed or not is_less_compressed: | ||
| pytest.skip( | ||
| f"Container size is not less than the published image {published_image_name}. " | ||
| f"Uncompressed image size: {is_less_uncopressed}, Compressed image size: {is_less_compressed}" | ||
| ) |
There was a problem hiding this comment.
Typo: is_less_uncopressed → is_less_uncompressed.
Local variable misspelling (missing m) repeated 3 times. Harmless but leaks into the skip message shown in CI logs.
Proposed fix
- is_less_uncopressed = ContainerCompareClass.is_uncompressed_image_smaller(
+ is_less_uncompressed = ContainerCompareClass.is_uncompressed_image_smaller(
built_image_name=VARS.IMAGE_NAME,
published_image=published_image_name,
)
is_less_compressed = ContainerCompareClass.is_compressed_image_smaller(
built_image_name=VARS.IMAGE_NAME,
published_image_name=published_image_name,
)
- if not is_less_uncopressed or not is_less_compressed:
+ if not is_less_uncompressed or not is_less_compressed:
pytest.skip(
f"Container size is not less than the published image {published_image_name}. "
- f"Uncompressed image size: {is_less_uncopressed}, Compressed image size: {is_less_compressed}"
+ f"Uncompressed image size: {is_less_uncompressed}, Compressed image size: {is_less_compressed}"
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/test_container_sizes.py` around lines 41 - 53, Rename the misspelled
local variable is_less_uncopressed to is_less_uncompressed everywhere it's used
in test/test_container_sizes.py: update the assignment from
ContainerCompareClass.is_uncompressed_image_smaller(...) to store into
is_less_uncompressed, update the conditional that checks it (if not
is_less_uncompressed or not is_less_compressed) and update the pytest.skip
message interpolation to reference is_less_uncompressed instead of
is_less_uncopressed so the variable name matches the intended meaning and avoids
the typo in CI logs.
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
1 participant
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.
Compare nginx version against the major OS version.
In case image is less then previous major OS version, then the test passed.
In case image is higher then previous major OS version, then the test is skipped.
Summary by CodeRabbit
Tests
Chores