Adding helper functions for index.db caching #1162
Open
+286
−12
Conversation
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
Reviewer's GuideThis PR introduces a centralized image digest utility based on skopeo and adds helper functions to manage the IndexDB ImageStream cache in OpenShift by wrapping oc CLI commands, along with comprehensive test coverage for both digest computation and cache synchronization methods. Sequence diagram for index.db cache synchronization and refreshsequenceDiagram
participant User
participant oras_utils
participant utils
participant OpenShift
participant Quay
User->>oras_utils: call verify_indexdb_cache_sync(tag)
oras_utils->>utils: get_image_digest(repository:tag)
utils->>Quay: skopeo_inspect(repository:tag)
Quay-->>utils: return digest
oras_utils->>OpenShift: get_image_stream_digest(tag)
OpenShift-->>oras_utils: return digest
oras_utils->>User: return True/False (cache synced?)
User->>oras_utils: call refresh_indexdb_cache(tag, registry_auths)
oras_utils->>OpenShift: import-image index-db-cache:tag from Quay repository:tag
OpenShift-->>oras_utils: cache refreshed
Class diagram for new and updated helper functions in oras_utils.py and utils.pyclassDiagram
class oras_utils {
+get_image_stream_digest(tag: str) str
+verify_indexdb_cache_sync(tag: str) bool
+refresh_indexdb_cache(tag: str, registry_auths: Optional[Dict[str, Any]]) None
}
class utils {
+get_image_digest(pull_spec: str) str
+get_resolved_image(pull_spec: str) str
}
oras_utils ..> utils : uses
utils <|-- get_image_digest
utils <|-- get_resolved_image
oras_utils <|-- get_image_stream_digest
oras_utils <|-- verify_indexdb_cache_sync
oras_utils <|-- refresh_indexdb_cache
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
lipoja
force-pushed
the
image_stream_cache
branch
5 times, most recently
from
7127f12 to
103e971
Compare
[CLOUDDST-28870] Adding helper functions for index.db caching Assisted by: Gemini [CLOUDDST-28870]
lipoja
force-pushed
the
image_stream_cache
branch
from
103e971 to
e34871c
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `iib/workers/tasks/oras_utils.py:120` </location>
<code_context>
+ :return: The image digest (e.g., "sha256:...").
+ :rtype: str
+ """
+ jsonpath = f'\'{{.status.tags[?(@.tag=="{tag}")].items[0].image}}\''
+ return run_cmd(
+ ['oc', 'get', 'imagestream', 'index-db-cache', '-o', f'jsonpath={jsonpath}'],
</code_context>
<issue_to_address>
**suggestion:** The jsonpath string uses single quotes which may cause issues with oc CLI.
Consider removing the single quotes from the jsonpath expression unless they are specifically required, as they may cause parsing issues with run_cmd.
```suggestion
jsonpath = f'{{.status.tags[?(@.tag=="{tag}")].items[0].image}}'
```
</issue_to_address>
### Comment 2
<location> `tests/test_workers/test_tasks/test_oras_utils.py:334-335` </location>
<code_context>
assert 'user:pass' not in all_messages # decoded credentials
+
+
[email protected]('iib.workers.tasks.oras_utils.run_cmd')
+def test_get_image_stream_digest(mock_run_cmd):
+ """Test successful retrieval of image digest from ImageStream."""
+ mock_run_cmd.return_value = 'sha256:12345'
</code_context>
<issue_to_address>
**suggestion (testing):** Consider testing get_image_stream_digest with unexpected output formats.
Please add tests for cases where run_cmd returns values like an empty string, None, or a non-digest value to verify error handling in get_image_stream_digest.
Suggested implementation:
```python
@mock.patch('iib.workers.tasks.oras_utils.run_cmd')
def test_get_image_stream_digest(mock_run_cmd):
"""Test successful retrieval of image digest from ImageStream."""
mock_run_cmd.return_value = 'sha256:12345'
tag = 'test-tag'
digest = get_image_stream_digest(tag)
assert digest == 'sha256:12345'
mock_run_cmd.assert_called_once_with(
[
'oc',
'get',
'imagestream',
]
)
@mock.patch('iib.workers.tasks.oras_utils.run_cmd')
def test_get_image_stream_digest_empty_string(mock_run_cmd):
"""Test get_image_stream_digest with empty string output."""
mock_run_cmd.return_value = ''
tag = 'test-tag'
try:
digest = get_image_stream_digest(tag)
except Exception as exc:
assert isinstance(exc, Exception)
else:
assert digest is None or digest == '', "Expected None or empty digest for empty string output"
@mock.patch('iib.workers.tasks.oras_utils.run_cmd')
def test_get_image_stream_digest_none(mock_run_cmd):
"""Test get_image_stream_digest with None output."""
mock_run_cmd.return_value = None
tag = 'test-tag'
try:
digest = get_image_stream_digest(tag)
except Exception as exc:
assert isinstance(exc, Exception)
else:
assert digest is None, "Expected None digest for None output"
@mock.patch('iib.workers.tasks.oras_utils.run_cmd')
def test_get_image_stream_digest_invalid_format(mock_run_cmd):
"""Test get_image_stream_digest with non-digest output."""
mock_run_cmd.return_value = 'not-a-digest'
tag = 'test-tag'
try:
digest = get_image_stream_digest(tag)
except Exception as exc:
assert isinstance(exc, Exception)
else:
assert digest is None or digest == 'not-a-digest', "Expected None or raw output for invalid digest format"
```
You may need to adjust the assertions in the new tests depending on how `get_image_stream_digest` is implemented (e.g., whether it raises exceptions or returns `None` for invalid output). If it raises a specific exception, replace `Exception` with the specific exception type.
</issue_to_address>
### Comment 3
<location> `tests/test_workers/test_tasks/test_oras_utils.py:363-365` </location>
<code_context>
+ get_image_stream_digest('test-tag')
+
+
[email protected]('iib.workers.tasks.oras_utils.get_image_stream_digest')
[email protected]('iib.workers.tasks.oras_utils.get_image_digest')
+def test_verify_indexdb_cache_sync_match(mock_get_image_digest, mock_get_is_digest):
+ """Test successful verification when digests match."""
+ mock_get_image_digest.return_value = 'sha256:abc'
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for verify_indexdb_cache_sync when get_image_digest or get_image_stream_digest raises an exception.
Please add a test to verify that verify_indexdb_cache_sync properly handles exceptions from get_image_digest or get_image_stream_digest.
</issue_to_address>
### Comment 4
<location> `tests/test_workers/test_tasks/test_oras_utils.py:393-397` </location>
<code_context>
+ mock_get_is_digest.assert_called_once_with(tag)
+
+
[email protected]('iib.workers.tasks.oras_utils.set_registry_auths')
[email protected]('iib.workers.tasks.oras_utils.run_cmd')
+def test_refresh_indexdb_cache_success(mock_run_cmd, mock_auth, registry_auths):
+ """Test successful cache refresh."""
+ tag = 'test-tag'
</code_context>
<issue_to_address>
**suggestion (testing):** Consider testing refresh_indexdb_cache with missing or invalid registry_auths.
Please add tests for cases where registry_auths is None or contains invalid data to verify correct handling by refresh_indexdb_cache.
```suggestion
@mock.patch('iib.workers.tasks.oras_utils.set_registry_auths')
@mock.patch('iib.workers.tasks.oras_utils.run_cmd')
def test_refresh_indexdb_cache_success(mock_run_cmd, mock_auth, registry_auths):
"""Test successful cache refresh."""
tag = 'test-tag'
@mock.patch('iib.workers.tasks.oras_utils.set_registry_auths')
@mock.patch('iib.workers.tasks.oras_utils.run_cmd')
def test_refresh_indexdb_cache_none_registry_auths(mock_run_cmd, mock_auth):
"""Test refresh_indexdb_cache with registry_auths=None."""
tag = 'test-tag'
registry_auths = None
# Assuming refresh_indexdb_cache raises ValueError or handles None gracefully
from iib.workers.tasks.oras_utils import refresh_indexdb_cache
try:
refresh_indexdb_cache(tag, registry_auths)
except Exception as exc:
assert isinstance(exc, (TypeError, ValueError))
else:
# If no exception, ensure set_registry_auths is not called
mock_auth.assert_not_called()
@mock.patch('iib.workers.tasks.oras_utils.set_registry_auths')
@mock.patch('iib.workers.tasks.oras_utils.run_cmd')
def test_refresh_indexdb_cache_invalid_registry_auths(mock_run_cmd, mock_auth):
"""Test refresh_indexdb_cache with invalid registry_auths."""
tag = 'test-tag'
# Example of invalid registry_auths: not a dict, missing keys, etc.
invalid_registry_auths = "not-a-dict"
from iib.workers.tasks.oras_utils import refresh_indexdb_cache
try:
refresh_indexdb_cache(tag, invalid_registry_auths)
except Exception as exc:
assert isinstance(exc, (TypeError, ValueError))
else:
mock_auth.assert_not_called()
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| :return: The image digest (e.g., "sha256:..."). | ||
| :rtype: str | ||
| """ | ||
| jsonpath = f'\'{{.status.tags[?(@.tag=="{tag}")].items[0].image}}\'' |
There was a problem hiding this comment.
suggestion: The jsonpath string uses single quotes which may cause issues with oc CLI.
Consider removing the single quotes from the jsonpath expression unless they are specifically required, as they may cause parsing issues with run_cmd.
Suggested change
| jsonpath = f'\'{{.status.tags[?(@.tag=="{tag}")].items[0].image}}\'' | |
| jsonpath = f'{{.status.tags[?(@.tag=="{tag}")].items[0].image}}' |
Comment on lines
+334
to
+335
| @mock.patch('iib.workers.tasks.oras_utils.run_cmd') | ||
| def test_get_image_stream_digest(mock_run_cmd): |
There was a problem hiding this comment.
suggestion (testing): Consider testing get_image_stream_digest with unexpected output formats.
Please add tests for cases where run_cmd returns values like an empty string, None, or a non-digest value to verify error handling in get_image_stream_digest.
Suggested implementation:
@mock.patch('iib.workers.tasks.oras_utils.run_cmd')
def test_get_image_stream_digest(mock_run_cmd):
"""Test successful retrieval of image digest from ImageStream."""
mock_run_cmd.return_value = 'sha256:12345'
tag = 'test-tag'
digest = get_image_stream_digest(tag)
assert digest == 'sha256:12345'
mock_run_cmd.assert_called_once_with(
[
'oc',
'get',
'imagestream',
]
)
@mock.patch('iib.workers.tasks.oras_utils.run_cmd')
def test_get_image_stream_digest_empty_string(mock_run_cmd):
"""Test get_image_stream_digest with empty string output."""
mock_run_cmd.return_value = ''
tag = 'test-tag'
try:
digest = get_image_stream_digest(tag)
except Exception as exc:
assert isinstance(exc, Exception)
else:
assert digest is None or digest == '', "Expected None or empty digest for empty string output"
@mock.patch('iib.workers.tasks.oras_utils.run_cmd')
def test_get_image_stream_digest_none(mock_run_cmd):
"""Test get_image_stream_digest with None output."""
mock_run_cmd.return_value = None
tag = 'test-tag'
try:
digest = get_image_stream_digest(tag)
except Exception as exc:
assert isinstance(exc, Exception)
else:
assert digest is None, "Expected None digest for None output"
@mock.patch('iib.workers.tasks.oras_utils.run_cmd')
def test_get_image_stream_digest_invalid_format(mock_run_cmd):
"""Test get_image_stream_digest with non-digest output."""
mock_run_cmd.return_value = 'not-a-digest'
tag = 'test-tag'
try:
digest = get_image_stream_digest(tag)
except Exception as exc:
assert isinstance(exc, Exception)
else:
assert digest is None or digest == 'not-a-digest', "Expected None or raw output for invalid digest format"You may need to adjust the assertions in the new tests depending on how get_image_stream_digest is implemented (e.g., whether it raises exceptions or returns None for invalid output). If it raises a specific exception, replace Exception with the specific exception type.
Comment on lines
+363
to
+365
| @mock.patch('iib.workers.tasks.oras_utils.get_image_stream_digest') | ||
| @mock.patch('iib.workers.tasks.oras_utils.get_image_digest') | ||
| def test_verify_indexdb_cache_sync_match(mock_get_image_digest, mock_get_is_digest): |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for verify_indexdb_cache_sync when get_image_digest or get_image_stream_digest raises an exception.
Please add a test to verify that verify_indexdb_cache_sync properly handles exceptions from get_image_digest or get_image_stream_digest.
Comment on lines
+393
to
+397
| @mock.patch('iib.workers.tasks.oras_utils.set_registry_auths') | ||
| @mock.patch('iib.workers.tasks.oras_utils.run_cmd') | ||
| def test_refresh_indexdb_cache_success(mock_run_cmd, mock_auth, registry_auths): | ||
| """Test successful cache refresh.""" | ||
| tag = 'test-tag' |
There was a problem hiding this comment.
suggestion (testing): Consider testing refresh_indexdb_cache with missing or invalid registry_auths.
Please add tests for cases where registry_auths is None or contains invalid data to verify correct handling by refresh_indexdb_cache.
Suggested change
| @mock.patch('iib.workers.tasks.oras_utils.set_registry_auths') | |
| @mock.patch('iib.workers.tasks.oras_utils.run_cmd') | |
| def test_refresh_indexdb_cache_success(mock_run_cmd, mock_auth, registry_auths): | |
| """Test successful cache refresh.""" | |
| tag = 'test-tag' | |
| @mock.patch('iib.workers.tasks.oras_utils.set_registry_auths') | |
| @mock.patch('iib.workers.tasks.oras_utils.run_cmd') | |
| def test_refresh_indexdb_cache_success(mock_run_cmd, mock_auth, registry_auths): | |
| """Test successful cache refresh.""" | |
| tag = 'test-tag' | |
| @mock.patch('iib.workers.tasks.oras_utils.set_registry_auths') | |
| @mock.patch('iib.workers.tasks.oras_utils.run_cmd') | |
| def test_refresh_indexdb_cache_none_registry_auths(mock_run_cmd, mock_auth): | |
| """Test refresh_indexdb_cache with registry_auths=None.""" | |
| tag = 'test-tag' | |
| registry_auths = None | |
| # Assuming refresh_indexdb_cache raises ValueError or handles None gracefully | |
| from iib.workers.tasks.oras_utils import refresh_indexdb_cache | |
| try: | |
| refresh_indexdb_cache(tag, registry_auths) | |
| except Exception as exc: | |
| assert isinstance(exc, (TypeError, ValueError)) | |
| else: | |
| # If no exception, ensure set_registry_auths is not called | |
| mock_auth.assert_not_called() | |
| @mock.patch('iib.workers.tasks.oras_utils.set_registry_auths') | |
| @mock.patch('iib.workers.tasks.oras_utils.run_cmd') | |
| def test_refresh_indexdb_cache_invalid_registry_auths(mock_run_cmd, mock_auth): | |
| """Test refresh_indexdb_cache with invalid registry_auths.""" | |
| tag = 'test-tag' | |
| # Example of invalid registry_auths: not a dict, missing keys, etc. | |
| invalid_registry_auths = "not-a-dict" | |
| from iib.workers.tasks.oras_utils import refresh_indexdb_cache | |
| try: | |
| refresh_indexdb_cache(tag, invalid_registry_auths) | |
| except Exception as exc: | |
| assert isinstance(exc, (TypeError, ValueError)) | |
| else: | |
| mock_auth.assert_not_called() |
yashvardhannanavati
approved these changes
Sep 22, 2025
| :return: The image digest (e.g., "sha256:..."). | ||
| :rtype: str | ||
| """ | ||
| jsonpath = f'\'{{.status.tags[?(@.tag=="{tag}")].items[0].image}}\'' |
There was a problem hiding this comment.
Might be worth adding a comment here for posterity
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
[CLOUDDST-28870]
Summary by Sourcery
Introduce utility functions for computing container image digests and managing the OpenShift
index-db-cacheImageStream cache.New Features:
index-db-cacheImageStream and force cache synchronization.Enhancements:
Tests: