Fix using rails-ex repo and rails-postgresql-persistent.json file#644
Open
phracek wants to merge 6 commits into
Open
Fix using rails-ex repo and rails-postgresql-persistent.json file#644phracek wants to merge 6 commits into
phracek wants to merge 6 commits into
Conversation
Pull Request validationFailed🔴 Failed or pending statuses:
🔴 Review - Missing review from a member (1 required) Triggered by Workflow Run |
Testing Farm results
|
Missing image definitions Signed-off-by: Petr "Stone" Hracek <[email protected]>
phracek
force-pushed
the
fix_rails_ex_and_remote
branch
from
ed720fb to
e043ffc
Compare
Member
Author
|
After fixing rails-ex repository and local JSON files. Let's test it again. [test-openshift-pytest] |
Signed-off-by: Petr "Stone" Hracek <[email protected]>
Signed-off-by: Petr "Stone" Hracek <[email protected]>
Member
Author
|
[test-openshift-pytest] |
The function is not needed at all. Signed-off-by: Petr "Stone" Hracek <[email protected]>
Member
Author
|
[test-openshift-pytest] |
📝 WalkthroughWalkthroughAdds Ruby 4.0 ImageStreamTags ( ChangesRuby 4.0 ImageStream support, template parameterization, and test updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
Signed-off-by: Petr "Stone" Hracek <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/test_ocp_s2i_imagestreams.py (1)
27-27:⚠️ Potential issue | 🟠 MajorRename this method to include the
test_prefix so pytest collects and executes it.The method at line 27 is defined as
ruby_deploy_imagestreamwithout thetest_prefix. Since it's part of theTestRubyImagestreamsclass and contains test assertions, pytest won't discover it without the prefix.Suggested fix
- def ruby_deploy_imagestream(self): + def test_ruby_deploy_imagestream(self):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_ocp_s2i_imagestreams.py` at line 27, The method ruby_deploy_imagestream in the TestRubyImagestreams class is missing the test_ prefix required by pytest for automatic test discovery. Rename the method from ruby_deploy_imagestream to test_ruby_deploy_imagestream so that pytest will discover and execute it as a test method.
🧹 Nitpick comments (1)
test/test_ocp_helm_ruby_imagestreams.py (1)
27-36: ⚡ Quick winExpand Helm imagestream assertions to include Ruby 4.0 tags.
Current parametrization validates 2.5/3.0/3.3 only, so this suite doesn’t verify the new
4.0-ubi9and4.0-ubi10contracts introduced by this PR.Suggested test update
`@pytest.mark.parametrize`( "version,registry,expected", [ + ("4.0-ubi10", "registry.redhat.io/ubi10/ruby-40:latest", True), + ("4.0-ubi9", "registry.redhat.io/ubi9/ruby-40:latest", True), ("3.3-ubi10", "registry.redhat.io/ubi10/ruby-33:latest", True), ("3.3-ubi9", "registry.redhat.io/ubi9/ruby-33:latest", True), ("3.3-ubi8", "registry.redhat.io/ubi8/ruby-33:latest", True), ("3.0-ubi9", "registry.redhat.io/ubi9/ruby-30:latest", True), ("3.0-ubi8", "registry.redhat.io/ubi8/ruby-30:latest", False), ("2.5-ubi8", "registry.redhat.io/ubi8/ruby-25:latest", True), ], )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_ocp_helm_ruby_imagestreams.py` around lines 27 - 36, The pytest.mark.parametrize decorator on the test function currently covers Ruby versions 2.5, 3.0, and 3.3 only, but this PR introduces new Ruby 4.0 contracts that are not being tested. Add two new test case tuples to the parametrization list for the 4.0-ubi10 and 4.0-ubi9 versions. Follow the same pattern as the existing entries: each tuple should include the version string (such as "4.0-ubi10" and "4.0-ubi9"), the corresponding registry.redhat.io image URL, and the expected boolean result for whether that imagestream is supported. This ensures the test suite validates the new Ruby 4.0 contracts introduced by this PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/test_ocp_s2i_imagestreams.py`:
- Line 27: The method ruby_deploy_imagestream in the TestRubyImagestreams class
is missing the test_ prefix required by pytest for automatic test discovery.
Rename the method from ruby_deploy_imagestream to test_ruby_deploy_imagestream
so that pytest will discover and execute it as a test method.
---
Nitpick comments:
In `@test/test_ocp_helm_ruby_imagestreams.py`:
- Around line 27-36: The pytest.mark.parametrize decorator on the test function
currently covers Ruby versions 2.5, 3.0, and 3.3 only, but this PR introduces
new Ruby 4.0 contracts that are not being tested. Add two new test case tuples
to the parametrization list for the 4.0-ubi10 and 4.0-ubi9 versions. Follow the
same pattern as the existing entries: each tuple should include the version
string (such as "4.0-ubi10" and "4.0-ubi9"), the corresponding
registry.redhat.io image URL, and the expected boolean result for whether that
imagestream is supported. This ensures the test suite validates the new Ruby 4.0
contracts introduced by this PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f4a1481-585d-41fd-a858-f38a1d755370
📒 Files selected for processing (11)
examples/rails-postgresql-persistent.jsonexamples/rails.jsonimagestreams/imagestreams.yamlimagestreams/ruby-centos.jsonimagestreams/ruby-rhel-aarch64.jsonimagestreams/ruby-rhel.jsontest/conftest.pytest/test_ocp_helm_ruby_imagestreams.pytest/test_ocp_helm_ruby_rails_application.pytest/test_ocp_s2i_imagestreams.pytest/test_ocp_s2i_templates.py
💤 Files with no reviewable changes (1)
- test/conftest.py
dependences installation. Do not check already removed imagestreams Signed-off-by: Petr "Stone" Hracek <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/test_ocp_helm_ruby_rails_application.py (1)
52-52: ⚡ Quick winCentralize the deployment timeout instead of repeating
480across tests.
The same wait threshold is now duplicated across multiple modules, which makes future tuning error-prone.
test/test_ocp_helm_ruby_rails_application.py#L52-L52: replace inlinetimeout=480with a shared constant.test/test_ocp_ruby_ex_standalone.py#L36-L36: use the same shared timeout constant.test/test_ocp_s2i_integration.py#L38-L38: use the same shared timeout constant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_ocp_helm_ruby_rails_application.py` at line 52, The timeout value 480 is hardcoded in multiple test files creating maintenance risk. Define a shared constant for the deployment timeout (e.g., DEPLOYMENT_TIMEOUT or WAIT_TIMEOUT) in a common location accessible to all tests, then replace the inline timeout=480 values in three locations: test/test_ocp_helm_ruby_rails_application.py line 52 (the is_s2i_pod_running call), test/test_ocp_ruby_ex_standalone.py line 36, and test/test_ocp_s2i_integration.py line 38. Each location should import and use the shared constant instead of the hardcoded numeric value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/test_ocp_helm_ruby_imagestreams.py`:
- Around line 27-36: The parametrized test matrix in the pytest.mark.parametrize
decorator is missing test coverage for the Ruby 4.0 UBI9 variant. Add a new
tuple to the version and registry parameter list following the same pattern as
the existing entries. The new tuple should include the version string "4.0-ubi9"
paired with the corresponding registry image URL
"registry.redhat.io/ubi9/ruby-40:latest" to ensure this advertised variant is
tested and regressions are caught in CI.
---
Nitpick comments:
In `@test/test_ocp_helm_ruby_rails_application.py`:
- Line 52: The timeout value 480 is hardcoded in multiple test files creating
maintenance risk. Define a shared constant for the deployment timeout (e.g.,
DEPLOYMENT_TIMEOUT or WAIT_TIMEOUT) in a common location accessible to all
tests, then replace the inline timeout=480 values in three locations:
test/test_ocp_helm_ruby_rails_application.py line 52 (the is_s2i_pod_running
call), test/test_ocp_ruby_ex_standalone.py line 36, and
test/test_ocp_s2i_integration.py line 38. Each location should import and use
the shared constant instead of the hardcoded numeric value.
🪄 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: 6517080d-6e78-4458-a7ca-2013504c6452
📒 Files selected for processing (9)
test/conftest.pytest/test_container_application.pytest/test_container_basics.pytest/test_ocp_helm_ruby_imagestreams.pytest/test_ocp_helm_ruby_rails_application.pytest/test_ocp_ruby_ex_standalone.pytest/test_ocp_s2i_imagestreams.pytest/test_ocp_s2i_integration.pytest/test_ocp_s2i_templates.py
💤 Files with no reviewable changes (2)
- test/test_container_application.py
- test/test_container_basics.py
🚧 Files skipped from review as they are similar to previous changes (3)
- test/test_ocp_s2i_imagestreams.py
- test/test_ocp_s2i_templates.py
- test/conftest.py
Comment on lines
27
to
36
| @pytest.mark.parametrize( | ||
| "version,registry,expected", | ||
| "version,registry", | ||
| [ | ||
| ("3.3-ubi10", "registry.redhat.io/ubi10/ruby-33:latest", True), | ||
| ("3.3-ubi9", "registry.redhat.io/ubi9/ruby-33:latest", True), | ||
| ("3.3-ubi8", "registry.redhat.io/ubi8/ruby-33:latest", True), | ||
| ("3.0-ubi9", "registry.redhat.io/ubi9/ruby-30:latest", True), | ||
| ("3.0-ubi8", "registry.redhat.io/ubi8/ruby-30:latest", False), | ||
| ("2.5-ubi8", "registry.redhat.io/ubi8/ruby-25:latest", True), | ||
| ("3.3-ubi10", "registry.redhat.io/ubi10/ruby-33:latest"), | ||
| ("3.3-ubi9", "registry.redhat.io/ubi9/ruby-33:latest"), | ||
| ("3.3-ubi8", "registry.redhat.io/ubi8/ruby-33:latest"), | ||
| ("3.0-ubi9", "registry.redhat.io/ubi9/ruby-30:latest"), | ||
| ("2.5-ubi8", "registry.redhat.io/ubi8/ruby-25:latest"), | ||
| ("4.0-ubi10", "registry.redhat.io/ubi10/ruby-40:latest"), | ||
| ], |
There was a problem hiding this comment.
Cover the missing Ruby 4.0 UBI9 imagestream tuple in this parametrized test.
This matrix validates newly supported tags, but it currently includes 4.0-ubi10 only. Add 4.0-ubi9 so regressions for that advertised variant are caught in CI.
Suggested patch
[
("3.3-ubi10", "registry.redhat.io/ubi10/ruby-33:latest"),
("3.3-ubi9", "registry.redhat.io/ubi9/ruby-33:latest"),
("3.3-ubi8", "registry.redhat.io/ubi8/ruby-33:latest"),
("3.0-ubi9", "registry.redhat.io/ubi9/ruby-30:latest"),
("2.5-ubi8", "registry.redhat.io/ubi8/ruby-25:latest"),
+ ("4.0-ubi9", "registry.redhat.io/ubi9/ruby-40:latest"),
("4.0-ubi10", "registry.redhat.io/ubi10/ruby-40:latest"),
],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/test_ocp_helm_ruby_imagestreams.py` around lines 27 - 36, The
parametrized test matrix in the pytest.mark.parametrize decorator is missing
test coverage for the Ruby 4.0 UBI9 variant. Add a new tuple to the version and
registry parameter list following the same pattern as the existing entries. The
new tuple should include the version string "4.0-ubi9" paired with the
corresponding registry image URL "registry.redhat.io/ubi9/ruby-40:latest" to
ensure this advertised variant is tested and regressions are caught in CI.
Member
Author
|
[test-openshift-pytest] |
Member
Author
|
[test-openshift-pytest] |
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.
Fix using rails-ex repo and rails-postgresql-persistent.json file
Missing image definitions
For first test let's use phracek fork
Summary by CodeRabbit
New Features
Tests