fix microshift could not determine rhel version from upstream#2494
fix microshift could not determine rhel version from upstream#2494fgallott wants to merge 1 commit intoopenshift-eng:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe PR enhances upstream image resolution by adding fallback mechanisms for RHEL version extraction from registry paths and implementing a Dockerfile modifications resolver that applies content.source.modifications to unresolved image references before re-parsing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@fgallott: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
![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
🤖 Fix all issues with AI agents
In `@doozer/tests/test_image.py`:
- Around line 857-898: The test_docstring for
test_determine_upstream_builder_info_unresolved_args_no_modifications is
incorrect: update the docstring to state that an IOError is expected when
upstream Dockerfile has unresolved ARG references and there are no
content.source.modifications (i.e., metadata._apply_alternative_upstream_config
should raise IOError rather than returning None for rhel_version); modify the
docstring text to reflect the expected exception and the scenario involving
unresolved ARGs and lack of modifications so it matches the assertion in the
test.
| def test_determine_upstream_builder_info_unresolved_args_no_modifications( | ||
| self, mock_joinpath, mock_open, mock_source_resolver | ||
| ): | ||
| """ | ||
| Test that when upstream Dockerfile has unresolved ARG references and there are no | ||
| content.source.modifications, the method returns None for rhel_version gracefully. | ||
| """ | ||
| extract_builder_info_from_pullspec.cache_clear() | ||
| metadata = self._create_image_metadata('openshift/test_no_mods') | ||
|
|
||
| metadata.config = Model( | ||
| { | ||
| 'name': 'openshift/test_no_mods', | ||
| 'distgit': {'branch': 'rhaos-4.18-rhel-9'}, | ||
| 'content': { | ||
| 'source': { | ||
| 'git': {'url': 'git@github.com:openshift-priv/test.git'}, | ||
| }, | ||
| }, | ||
| } | ||
| ) | ||
|
|
||
| metadata.branch_el_target = MagicMock(return_value=9) | ||
|
|
||
| mock_source_resolution = MagicMock() | ||
| metadata.runtime.source_resolver.resolve_source = MagicMock(return_value=mock_source_resolution) | ||
| mock_source_dir = MagicMock() | ||
| mock_source_resolver.get_source_dir = MagicMock(return_value=mock_source_dir) | ||
| mock_joinpath.return_value = MagicMock() | ||
|
|
||
| dockerfile_content = "ARG BASE_IMAGE_URL\nARG BASE_IMAGE_TAG\nFROM ${BASE_IMAGE_URL}:${BASE_IMAGE_TAG}\n" | ||
| mock_open.return_value.__enter__.return_value.read.return_value = dockerfile_content | ||
|
|
||
| with patch('doozerlib.image.DockerfileParser') as mock_dfp_cls: | ||
| mock_dfp_instance = MagicMock() | ||
| mock_dfp_instance.parent_images = [':'] | ||
| mock_dfp_cls.return_value = mock_dfp_instance | ||
|
|
||
| # Should raise IOError because rhel_version could not be determined | ||
| with self.assertRaises(IOError): | ||
| metadata._apply_alternative_upstream_config() | ||
|
|
There was a problem hiding this comment.
Docstring contradicts expected behavior.
The test expects an IOError, but the docstring says it returns None for rhel_version. Please align the docstring with the assertion.
✏️ Suggested docstring fix
- Test that when upstream Dockerfile has unresolved ARG references and there are no
- content.source.modifications, the method returns None for rhel_version gracefully.
+ Test that when upstream Dockerfile has unresolved ARG references and there are no
+ content.source.modifications, the method raises IOError.🤖 Prompt for AI Agents
In `@doozer/tests/test_image.py` around lines 857 - 898, The test_docstring for
test_determine_upstream_builder_info_unresolved_args_no_modifications is
incorrect: update the docstring to state that an IOError is expected when
upstream Dockerfile has unresolved ARG references and there are no
content.source.modifications (i.e., metadata._apply_alternative_upstream_config
should raise IOError rather than returning None for rhel_version); modify the
docstring text to reflect the expected exception and the scenario involving
unresolved ARGs and lack of modifications so it matches the assertion in the
test.
| return None | ||
|
|
||
| pullspec = parent_images[-1] | ||
| if not pullspec or "${" in pullspec or pullspec == ":": |
There was a problem hiding this comment.
Not strictly needed, but defining something like is_pullspec_unresolved(pullspec: str) would make the code more readable and maintainable (also seen at line 899)
|
/lgtm Left an optional comment |
2 participants
working build