fix(workbenches): the way we check internal image registry state #499
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactored the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/cherry-pick', '/build-push-pr-image', '/wip', '/verified', '/lgtm', '/hold'} |
jstourac
force-pushed
the
fixWorkbenchRegistry
branch
2 times, most recently
from
ba8e89f to
be8a26d
Compare
![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
🔭 Outside diff range comments (1)
tests/workbenches/conftest.py (1)
91-96: Bug: invalid image reference when internal registry is disabled.
Else branch builds only a tag (e.g., ":2025.1"), which is not a valid image reference. This will fail image pulls. Use the full minimal_image instead.minimal_image_path = ( f"{INTERNAL_IMAGE_REGISTRY_PATH}/{py_config['applications_namespace']}/{minimal_image}" if internal_image_registry - else ":" + minimal_image.rsplit(":", maxsplit=1)[1] + else minimal_image )If a different external registry is intended, wire its fully qualified reference here instead of a plain tag.
♻️ Duplicate comments (1)
tests/workbenches/conftest.py (1)
50-51: Prefer using the wrapper’s Config resource over raw DynamicClient.
Now that the wrapper supports the imageregistry Config (per earlier discussion), use it instead of dynamic discovery for better typing and readability.Example (adjust import to your available wrapper class/module):
# from ocp_resources.<imageregistry_config_module> import Config cfg = Config(client=admin_client, name="cluster") management_state = str((getattr(cfg.instance, "spec", {}) or {}).get("managementState", "")).lower()
🧹 Nitpick comments (1)
tests/workbenches/conftest.py (1)
17-19: Consolidate constants imports and usage for consistency.
You import Labels directly but still use constants.Labels elsewhere. Suggest switching all uses to the direct Labels import and drop the utilities.constants module import to reduce duplication.Diff to remove the extra module import:
-from utilities import constantsOutside-of-range change to use the direct Labels import (keeps behavior):
- label={constants.Labels.OpenDataHub.DASHBOARD: "true"}, + label={Labels.OpenDataHub.DASHBOARD: "true"},
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/workbenches/conftest.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
📚 Learning: 2025-06-05T10:05:17.642Z
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Applied to files:
tests/workbenches/conftest.py
🧬 Code Graph Analysis (1)
tests/workbenches/conftest.py (2)
utilities/constants.py (1)
Labels(180-209)tests/conftest.py (1)
admin_client(66-67)
🔇 Additional comments (2)
tests/workbenches/conftest.py (2)
6-6: Good move adopting the shared module logger.
Keeps logging consistent across tests. No issues.Also applies to: 21-21
74-74: Fixture type update is correct.
Updating default_notebook to accept internal_image_registry: bool matches the refactored fixture. Good.
jstourac
force-pushed
the
fixWorkbenchRegistry
branch
from
be8a26d to
6a9dc1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/workbenches/conftest.py (1)
91-96: Bug: invalid image reference when registry is unavailableElse branch builds ":{tag}" (e.g., ":2025.1"), which is not a valid container image reference. Use the imagestream/tag value (minimal_image) instead.
- minimal_image_path = ( - f"{INTERNAL_IMAGE_REGISTRY_PATH}/{py_config['applications_namespace']}/{minimal_image}" - if internal_image_registry - else ":" + minimal_image.rsplit(":", maxsplit=1)[1] - ) + minimal_image_path = ( + f"{INTERNAL_IMAGE_REGISTRY_PATH}/{py_config['applications_namespace']}/{minimal_image}" + if internal_image_registry + else minimal_image + )
♻️ Duplicate comments (2)
tests/workbenches/conftest.py (2)
46-52: Prefer openshift-python-wrapper resource over raw DynamicClient (once available)Per prior review, wrapper support for imageregistry Config landed; consider switching to the wrapper resource (Config(name="cluster")) for consistency and stronger typing once a release is available.
53-60: Harden spec extraction and tighten exception handlingAvoid AttributeError if spec is missing and log stack traces on unexpected errors.
Apply:
- management_state = config_instance.spec.get("managementState", "").lower() - is_available = management_state == "managed" - - LOGGER.info(f"Image registry management state: {management_state}, available: {is_available}") - yield is_available - except (ResourceNotFoundError, Exception) as e: - LOGGER.warning(f"Failed to check image registry config: {e}") - yield False + spec = getattr(config_instance, "spec", None) or {} + management_state = str(spec.get("managementState", "")).lower() + is_available = management_state == "managed" + + LOGGER.info(f"Image registry management state: {management_state}, available: {is_available}") + yield is_available + except ResourceNotFoundError as e: + LOGGER.warning(f"Image registry config not found: {e}") + yield False + except Exception: + LOGGER.exception("Failed to check image registry config") + yield False
🧹 Nitpick comments (1)
tests/workbenches/conftest.py (1)
17-17: Use Labels consistently (avoid mixed imports)You now import Labels directly but still reference constants.Labels elsewhere. Consider standardizing on one style for consistency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/workbenches/conftest.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
📚 Learning: 2025-06-05T10:05:17.642Z
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Applied to files:
tests/workbenches/conftest.py
🧬 Code Graph Analysis (1)
tests/workbenches/conftest.py (2)
utilities/constants.py (1)
Labels(180-209)tests/conftest.py (1)
admin_client(66-67)
🔇 Additional comments (4)
tests/workbenches/conftest.py (4)
6-6: Logger import LGTMUsing simple_logger’s get_logger aligns with project conventions.
21-21: Module-level LOGGER initialization LGTMGood addition; keeps log statements consistent and contextual.
74-75: Param type update in default_notebook LGTMMatches the fixture’s new boolean contract.
43-45: internal_image_registry fixture usage verified
All downstream references have been updated to expect abool, and no tests or type hints remain targeting the old resource object. No further changes required.• tests/workbenches/conftest.py: lines 74, 94–96
The original method wasn't reliable for our infrastructure as we have the imageregistry ClusterOperator present in all our clusters by default. What differs is the actual management status of them, so this new approach should work fine for us now.
jstourac
force-pushed
the
fixWorkbenchRegistry
branch
from
6a9dc1e to
bcbe77d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/workbenches/conftest.py (3)
6-6: Good switch to module-level LOGGER via simple_logger.Resolves the prior suggestion to standardize logging.
Also applies to: 22-22
16-16: Correct usage of the ocp_resources wrapper for imageregistry Config.Using Config(name="cluster") from the wrapper is the right approach and aligns with prior feedback.
47-57: Fix spec extraction (AttributeError risk) and tighten exception handling.config_instance.instance.spec is a ResourceInstance field, not a dict; calling .get will likely raise AttributeError. Also, split exception handling and log stack traces for unexpected errors.
Apply:
- # Access the imageregistry.operator.openshift.io/v1 Config resource named "cluster" - config_instance = Config(client=admin_client, name="cluster") - - management_state = config_instance.instance.spec.get("managementState", "").lower() - is_available = management_state == "managed" - - LOGGER.info(f"Image registry management state: {management_state}, available: {is_available}") - yield is_available - except (ResourceNotFoundError, Exception) as e: - LOGGER.warning(f"Failed to check image registry config: {e}") - yield False + # Access the imageregistry.operator.openshift.io/v1 Config resource named "cluster" + config_instance = Config(client=admin_client, name="cluster") + + # Ensure the resource exists and safely extract spec + if not config_instance.exists: + LOGGER.warning("Image registry Config 'cluster' not found") + yield False + return + + cfg = getattr(getattr(config_instance, "instance", None), "to_dict", lambda: {})() + spec = (cfg or {}).get("spec", {}) or {} + management_state = str(spec.get("managementState", "")).lower() + is_available = management_state == "managed" + + LOGGER.info(f"Image registry management state: {management_state}, available: {is_available}") + yield is_available + except ResourceNotFoundError as e: + LOGGER.warning(f"Image registry config not found: {e}") + yield False + except Exception: + # Include stack trace for unexpected failures + LOGGER.exception("Failed to check image registry config") + yield False
🧹 Nitpick comments (1)
tests/workbenches/conftest.py (1)
18-18: Consistency nit: unify Labels import usage across the file.You import Labels directly but still reference constants.Labels elsewhere. Pick one style for readability (prefer the direct Labels import).
Example change outside this hunk:
- Line 33: replace constants.Labels.OpenDataHub.DASHBOARD with Labels.OpenDataHub.DASHBOARD
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/workbenches/conftest.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
📚 Learning: 2025-06-05T10:05:17.642Z
Learnt from: adolfo-ab
PR: opendatahub-io/opendatahub-tests#334
File: tests/model_explainability/trustyai_service/test_trustyai_service.py:52-65
Timestamp: 2025-06-05T10:05:17.642Z
Learning: For TrustyAI image validation tests: operator image tests require admin_client, related_images_refs, and trustyai_operator_configmap fixtures, while service image tests would require different fixtures like trustyai_service_with_pvc_storage, model_namespace, and current_client_token.
Applied to files:
tests/workbenches/conftest.py
🔇 Additional comments (2)
tests/workbenches/conftest.py (2)
71-73: Consumer fixture signature updated to bool — LGTM.default_notebook now depends on a boolean flag. This aligns with the new internal_image_registry contract.
44-46: internal_image_registry fixture returns bool – all call sites align
Verified thatinternal_image_registryis only used as a boolean in tests/workbenches/conftest.py (e.g.if internal_image_registry) and no attribute‐style calls remain. Approving the change.
|
ready for another review round, thanks guys! |
|
Status of building tag latest: success. |
The original method wasn't reliable for our infrastructure as we have the imageregistry ClusterOperator present in all our clusters by default. What differs is the actual management status of them, so this new approach should work fine for us now.
I also added a small logging information.
How Has This Been Tested?
worked on clusters both with and without internal image registry enabled
Merge criteria:
Waiting for the #510.
We should then backport this into 2.23 branch at least.
Backports: