Skip to content

feat: add aap_job_template_id support to Hook class #2759

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AmenB wants to merge 1 commit into
base: main
Choose a base branch
from
+32 −39
Open

feat: add aap_job_template_id support to Hook class #2759

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 32 additions & 39 deletions ocp_resources/hook.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,49 +1,42 @@
from ocp_resources.resource import NamespacedResource
from ocp_resources.utils.constants import TIMEOUT_4MINUTES
import warnings
from typing import Any

from ocp_resources._hook_generated import Hook as _GeneratedHook

class Hook(NamespacedResource):
"""
Migration Tool for Virtualization (MTV) Plan's Hook Resource.
_UNSET = object()
_DEFAULT_IMAGE = "quay.io/konveyor/hook-runner:latest"


class Hook(_GeneratedHook):
"""
Deprecated shim for backward compatibility.

api_group = NamespacedResource.ApiGroup.FORKLIFT_KONVEYOR_IO
Preserves the legacy default-image behavior. New code should use
``from ocp_resources._hook_generated import Hook`` directly.
"""

def __init__(
self,
name=None,
namespace=None,
image="quay.io/konveyor/hook-runner:latest",
playbook=None,
client=None,
teardown=True,
yaml_file=None,
delete_timeout=TIMEOUT_4MINUTES,
**kwargs,
):
"""
Args:
image (str): Path to an ansible image
playbook (str): Ansible playbook to be performed
"""
aap: dict[str, Any] | None = None,
deadline: int | None = None,
image: Any = _UNSET,
playbook: str | None = None,
service_account: str | None = None,
**kwargs: Any,
) -> None:
warnings.warn(
"Hook legacy defaults are deprecated and will be removed. "
"Pass image= explicitly for local hooks; use aap={...} for AAP hooks.",
DeprecationWarning,
stacklevel=2,
)
if image is _UNSET:
image = None if aap is not None else _DEFAULT_IMAGE
Comment on lines +27 to +34

@coderabbitai coderabbitai Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Move the deprecation warning inside the unset check and add the missing TODO.

Currently, the DeprecationWarning is emitted unconditionally. As pointed out in previous reviews, this means callers who explicitly pass image="quay.io/konveyor/hook-runner:latest" (or any other image) will still receive a warning instructing them to pass image=, which they already did. Additionally, the requested TODO comment to remove the fallback is missing.

Move the warning inside the if image is _UNSET: block to only warn when the legacy default behavior is invoked, and add the TODO comment.

💡 Proposed fix 
-        warnings.warn(
-            "Hook legacy defaults are deprecated and will be removed. "
-            "Pass image= explicitly for local hooks; use aap={...} for AAP hooks.",
-            DeprecationWarning,
-            stacklevel=2,
-        )
         if image is _UNSET:
+            warnings.warn(
+                "Hook legacy defaults are deprecated and will be removed. "
+                "Pass image= explicitly for local hooks; use aap={...} for AAP hooks.",
+                DeprecationWarning,
+                stacklevel=2,
+            )
+            # TODO: remove this fallback once the `image` default is fully deprecated
             image = None if aap is not None else _DEFAULT_IMAGE
🤖 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 `@ocp_resources/hook.py` around lines 27 - 34, In the hook initialization
logic, move the DeprecationWarning into the `if image is _UNSET:` block so
explicitly supplied images do not trigger it. Add the requested TODO in that
block documenting removal of the legacy fallback, while preserving the existing
`aap`-dependent image assignment.

super().__init__(
name=name,
namespace=namespace,
client=client,
teardown=teardown,
yaml_file=yaml_file,
delete_timeout=delete_timeout,
aap=aap,
deadline=deadline,
image=image,
playbook=playbook,
service_account=service_account,
**kwargs,
)
self.image = image
self.playbook = playbook

def to_dict(self) -> None:
super().to_dict()
if not self.kind_dict and not self.yaml_file:
self.res.update({
"spec": {
"image": self.image,
"playbook": self.playbook,
},
})