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
+18 −6
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
24 changes: 18 additions & 6 deletions ocp_resources/hook.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import warnings

from ocp_resources.resource import NamespacedResource
from ocp_resources.utils.constants import TIMEOUT_4MINUTES

Expand All @@ -15,6 +17,7 @@ def __init__(
namespace=None,
image="quay.io/konveyor/hook-runner:latest",
playbook=None,
aap_job_template_id=None,
client=None,
teardown=True,
yaml_file=None,
Comment on lines 17 to 23

@qodo-code-review qodo-code-review Bot Jul 9, 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.

Action required

1. Positional args api break 🐞 Bug ≡ Correctness

Hook.__init__ inserts aap_job_template_id before client, changing the positional argument
order; any existing positional calls that previously passed client (or later args) will now bind
that value to aap_job_template_id and leave client=None. This can lead to incorrect Hook
manifests (e.g., non-serializable jobTemplateId) and unexpected client initialization behavior.
Agent Prompt 
## Issue description
`Hook.__init__` added `aap_job_template_id` in the middle of the positional parameters (between `playbook` and `client`). This breaks backward compatibility for any callers using positional arguments, because the value that used to be `client` will now be interpreted as `aap_job_template_id`.

## Issue Context
This project exposes resource wrapper classes as a library; constructor signatures are part of the public API. Adding a new parameter in the middle is a silent behavioral change for positional callers.

## Fix Focus Areas
- ocp_resources/hook.py[12-24]

## Implementation guidance
- Make `aap_job_template_id` keyword-only by inserting `*` before it, e.g.:
  - `def __init__(..., playbook=None, *, aap_job_template_id=None, client=None, ...)`
  This preserves positional compatibility for existing code while still allowing the new feature.
- Alternatively, move `aap_job_template_id` after `yaml_file`/`delete_timeout` (end of the explicit params) and/or require it to be passed by keyword.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Expand All @@ -25,6 +28,7 @@ def __init__(
Args:
image (str): Path to an ansible image
playbook (str): Ansible playbook to be performed
aap_job_template_id (int): AAP/AWX job template ID to trigger during hook execution
"""
super().__init__(
name=name,
Expand All @@ -35,15 +39,23 @@ def __init__(
delete_timeout=delete_timeout,
**kwargs,
)
if image == "quay.io/konveyor/hook-runner:latest":
warnings.warn(
"The default value for 'image' will be removed in the next release. Pass 'image' explicitly.",
DeprecationWarning,
stacklevel=2,
)
self.image = image
self.playbook = playbook
self.aap_job_template_id = aap_job_template_id
Comment on lines 1 to +50

@myakove myakove Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Use a sentinel to distinguish "caller didn't pass image" from "caller explicitly passed the default value." String comparison can't tell the two apart — someone calling Hook(image="quay.io/konveyor/hook-runner:latest") explicitly still gets warned.

Suggested change
_UNSET = object()
if image is _UNSET:
warnings.warn(
"The default value for 'image' will be removed in the next release. Pass 'image' explicitly.",
DeprecationWarning,
stacklevel=2,
)
image = "quay.io/konveyor/hook-runner:latest" if aap_job_template_id is None else None


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,
},
})
self.res["spec"] = {}
_spec = self.res["spec"]
if self.aap_job_template_id is not None:

@myakove myakove Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we have here if;else?

@myakove myakove Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

and if you already modify the code here, switch to use the standatd approch for doing this
self.res["spec"] = {}
_spec = self.res["spec"]
if self.aap_job_template_id is not None:
_spec[jobTemplateId] = self.aap_job_template_id

for both please

@AmenB AmenB Jul 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The Hook CRD supports two mutually exclusive modes — playbook-based (spec.image + spec.playbook) and
AAP-based (spec.aap.jobTemplateId). They can't coexist in the same CR, so the if/else picks which spec
shape to emit

@myakove myakove Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

we do not block anything in code level, if it can sent to ocp api we send it, the ocp api will give the error if there is one.

@AmenB AmenB Jul 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The if/else is needed here because image has a default value ("quay.io/konveyor/hook-runner:latest"), so it's always truthy. Without the if/else, AAP hooks end up with both spec.aap.jobTemplateId and spec.image in the CR — Forklift sees both and the Plan never reaches Ready.

We tested locally without the if/else and confirmed it fails. The two hook types have different spec shapes — it's not validation, just picking which fields to emit based on what the caller asked for.

I did switch to the standard _spec pattern as you suggested.

@myakove myakove Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@rnetser thinkg of removing the defult image, WDYT?

@myakove myakove Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@AmenB for now keep the if else and add a detraction warn about the image default will be gone in next release

@AmenB AmenB Jul 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@myakove Added the deprecation warning. Note that it'll fire for every playbook hook that doesn't pass image explicitly — could be noisy. Want a different approach?

_spec["aap"] = {"jobTemplateId": self.aap_job_template_id}
else:
_spec["image"] = self.image
_spec["playbook"] = self.playbook