feat: add aap_job_template_id support to Hook class

[AmenB]

Summary

Add aap_job_template_id parameter to the Hook class, enabling MTV AAP hook integration — Hook CRs that trigger AWX/AAP job templates during migration instead of running playbooks in containers.

What changed

• New parameter: aap_job_template_id=None on Hook.__init__().
• Branching logic in to_dict():
  • When aap_job_template_id is set → produces spec.aap.jobTemplateId.
  • Otherwise → produces spec.image + spec.playbook (existing behavior, unchanged).

No regression — existing callers that don't pass aap_job_template_id hit the original code path.

Why

The mtv-api-tests project (branch feat/aap-hook-integration) has a new AAP hook integration test that creates a Hook CR via Hook(aap_job_template_id=9). Without this wrapper change, it fails with:

TypeError: Resource.__init__() got an unexpected keyword argument 'aap_job_template_id'

Confirmed by running the test against the current PyPI release (11.0.134).

Related Jira tickets

• MTV-6031: [Automation] AAP Hook Integration test
• MTV-3663: [TP] MTV hook for AAP integration (feature epic)
• MTV-6063: AAP hook migration hangs on TLS error (bug found during testing)

Polarion: MTV-781

Summary by CodeRabbit

• New Features
  • Added an optional AAP job template ID setting for Hook resources.
  • Hook serialization can now emit spec.aap.jobTemplateId when a job template ID is provided.
• Behavior Changes
  • If no job template ID is set, existing Hook output continues to use the prior spec.image/playbook structure.
• Deprecations
  • A deprecation warning is shown when the Hook image is omitted (so the default is applied only when no job template ID is provided).

[redhat-qe-bot]

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

• Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
• Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
• Issue Creation: Disabled for this repository
• Branch Labeling: Branch-specific labels are applied to track the target branch
• Auto-verification: Auto-verified users have their PRs automatically marked as verified
• Labels: All label categories are enabled (default configuration)

📋 Available Commands

PR Status Management

• /wip - Mark PR as work in progress (adds WIP: prefix to title)
• /wip cancel - Remove work in progress status
• /hold - Block PR merging (approvers only)
• /hold cancel - Unblock PR merging
• /verified - Mark PR as verified
• /verified cancel - Remove verification status
• /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)
• /regenerate-welcome - Regenerate this welcome message
• /security-override - Set security check runs to pass (maintainers only)
• /security-override cancel - Re-run security checks

Review & Approval

• /lgtm - Approve changes (looks good to me)
• /approve - Approve PR (approvers only)
• /automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
• /assign-reviewers - Assign reviewers based on OWNERS file
• /assign-reviewer @username - Assign specific reviewer
• /check-can-merge - Check if PR meets merge requirements

Testing & Validation

• /retest tox - Run Python test suite with tox
• /retest python-module-install - Test Python package installation
• /retest conventional-title - Validate commit message format
• /retest all - Run all available tests

Cherry-pick Operations

• /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
  • Multiple branches: /cherry-pick branch1 branch2 branch3
• /cherry-pick-retry <branch> - Retry a failed cherry-pick (merged PRs only)

Branch Management

• /rebase - Rebase this PR branch onto its base branch

Label Management

• /<label-name> - Add a label to the PR
• /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

1. Approval: /approve from at least one approver
2. Status Checks: All required status checks must pass
3. No Blockers: No wip, hold, has-conflicts labels and PR must be mergeable (no conflicts)
4. Verified: PR must be marked as verified

📊 Review Process

Approvers and Reviewers

Approvers:

• myakove
• rnetser

Reviewers:

• myakove
• rnetser

Available Labels

• hold
• verified
• wip
• lgtm
• approve
• automerge

AI Features

• Conventional Title: Mode: fix (claude/claude-opus-4-6-1m)
• Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6-1m)

Security Checks

• Suspicious Path Detection: Monitors paths: .claude/, .vscode/, .cursor/, .devcontainer/, .pi/, .github/workflows/, .github/actions/
• Committer Identity Check: Verifies last committer matches PR author
• Mandatory: Security checks block merge (use /security-override to bypass — maintainers only)

💡 Tips

• WIP Status: Use /wip when your PR is not ready for review
• Verification: The verified label is removed on new commits unless the push is detected as a clean rebase
• Cherry-picking: Cherry-pick labels are processed when the PR is merged
• Permission Levels: Some commands require approver permissions
• Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

[coderabbitai]

Warning

Review limit reached

@AmenB, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 25 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c58bdc67-9ac9-4d1e-9627-b628a9877a78

📥 Commits

Reviewing files that changed from the base of the PR and between 517e05f and 0510388.

📒 Files selected for processing (1)

• ocp_resources/hook.py

Walkthrough

Hook now accepts an optional aap_job_template_id, distinguishes omitted images from explicit values, warns when the image is omitted, and selects either AAP job template or image/playbook fields during serialization.

Changes

Hook AAP job template support

Layer / File(s)	Summary
Constructor parameter and default-image handling ocp_resources/hook.py	Adds and documents aap_job_template_id, stores it on the instance, detects omitted image values, emits a DeprecationWarning, and applies the default image only when no AAP job template is provided.
Conditional spec serialization ocp_resources/hook.py	to_dict() emits spec.aap.jobTemplateId for AAP-backed hooks or the existing spec.image/spec.playbook structure otherwise, when kind_dict and yaml_file are unset.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately summarizes the main change: adding aap_job_template_id support to Hook.
Description check	✅ Passed	It covers the feature, behavior change, motivation, and related issues, but doesn't use the exact template headings and omits Bug/Special notes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Add AAP/AWX job template support to MTV Hook resource

✨ Enhancement 🕐 10-20 Minutes

AI Description

• Add optional aap_job_template_id to Hook for AAP/AWX-backed hooks.
• Branch Hook.to_dict() to emit spec.aap.jobTemplateId when provided.
• Preserve existing image/playbook Hook spec when AAP job template is unset.

Diagram

graph TD
  A["Caller/Test"] --> B["Hook.__init__"] --> C{"aap_job_template_id set?"}
  C -- "yes" --> D["Hook spec: aap.jobTemplateId"]
  C -- "no" --> E["Hook spec: image"] --> F["Hook spec: playbook"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Validate mutual exclusivity in __init__

• ➕ Fails fast when both AAP and image/playbook inputs are provided
• ➕ Makes API contract explicit and avoids ambiguous spec generation
• ➖ Potential behavior change for any callers currently passing both (even if unintended)

2. Introduce a dedicated AAPHook subclass/factory

• ➕ Cleaner separation of the two hook modes and their required fields
• ➕ Easier to extend AAP-specific options later (credentials, controller URL, etc.)
• ➖ More surface area (new class/function) for a small feature
• ➖ May be overkill if only one AAP field is needed short-term

3. Allow passing a raw spec override (advanced usage)

• ➕ Maximum flexibility without adding parameters for every new spec field
• ➕ Useful for future MTV Hook spec expansions
• ➖ Weaker typing/validation; easier to build invalid CRs
• ➖ Harder to keep consistent behavior across callers

Recommendation: The current approach (optional aap_job_template_id with a to_dict() branch) is a good, low-risk, backward-compatible way to unblock AAP integration. Consider adding a simple mutual-exclusion validation (and ideally a small unit test) to make the ‘AAP vs image/playbook’ contract explicit and prevent accidental mixed inputs.

Files changed (1) +18 / -6

Enhancement (1) +18 / -6

hook.pyAdd AAP job template option and branch Hook spec serialization +18/-6

Add AAP job template option and branch Hook spec serialization

• Adds an optional aap_job_template_id parameter to Hook and stores it on the instance. Updates to_dict() to emit spec.aap.jobTemplateId when set, otherwise preserves the existing spec.image/spec.playbook output.

ocp_resources/hook.py

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Positional args API break 🐞 Bug ≡ Correctness

Description

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.

Code

ocp_resources/hook.py[R15-21]

        namespace=None,
        image="quay.io/konveyor/hook-runner:latest",
        playbook=None,
+        aap_job_template_id=None,
        client=None,
        teardown=True,
        yaml_file=None,

Relevance

⭐⭐⭐ High

They fix API contract breaks to avoid runtime misbinding/TypeError (see signature fix in PR #2574).

PR-#2574
PR-#2623

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The constructor signature shows aap_job_template_id was added before client, so any positional
argument previously intended for client will now be assigned to aap_job_template_id.

ocp_resources/hook.py[12-23]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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

---

2. Unvalidated mixed hook modes 🐞 Bug ☼ Reliability

Description

When aap_job_template_id is set, Hook.to_dict() emits only spec.aap.jobTemplateId and omits
spec.image/spec.playbook even if the caller provided them, with no validation to prevent/flag
mixed configuration. This can silently produce a manifest missing expected fields, making
misconfigurations harder to detect.

Code

ocp_resources/hook.py[R47-61]

+            if self.aap_job_template_id is not None:
+                self.res.update({
+                    "spec": {
+                        "aap": {
+                            "jobTemplateId": self.aap_job_template_id,
+                        },
+                    },
+                })
+            else:
+                self.res.update({
+                    "spec": {
+                        "image": self.image,
+                        "playbook": self.playbook,
+                    },
+                })

Relevance

⭐⭐ Medium

Exclusivity/precedence fixes accepted in PR #2480, but added validations often rejected (e.g., PR
#2520).

PR-#2480
PR-#2520

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

to_dict() branches solely on aap_job_template_id is not None; in that branch it constructs
spec with only aap.jobTemplateId and does not include image/playbook at all.

ocp_resources/hook.py[12-21]
ocp_resources/hook.py[44-61]
PR-#2480

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Hook` currently accepts `image`/`playbook` and `aap_job_template_id` simultaneously, but `to_dict()` will silently prefer the AAP output and drop `image`/`playbook` from the generated manifest. This makes mixed configuration errors easy to miss.

## Issue Context
The constructor has defaults for `image` and an optional `playbook`, so callers may set these while also experimenting with `aap_job_template_id`. Today this yields an output manifest that discards part of the provided configuration without warning.

## Fix Focus Areas
- ocp_resources/hook.py[25-61]

## Implementation guidance
- Add explicit validation in `__init__` (preferred) or `to_dict()`:
 - If `aap_job_template_id is not None` and (`playbook is not None` or `image` was explicitly set), raise `ValueError` explaining the options are mutually exclusive (or document/encode precedence).
 - Optionally, require `playbook` to be non-None when `aap_job_template_id is None` (if the underlying CRD requires it), but only if you can confirm this requirement.
- Update docstring to clearly state exclusivity/precedence.

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

---

[AmenB]

/verified

[redhat-qe-bot]

AmenB is not allowed to run commands.
maintainers can allow it by comment /add-allowed-user @AmenB
Maintainers:

• jsamato
• ogajduse
• redhat-qe-bot1
• smallamp
• omaciel
• thrix
• psav
• dbasunag
• RonnyPfannschmidt
• rnetser
• Aftermath
• myakove-bot
• dajohnso
• jkandasa
• rh-bot-1
• FilipB
• spoore1
• redhat-qe-bot
• Lorquas
• myakove
• quality-innovators-bot
• etirta
• redhat-qe-bot2
• mshriver
• rlandy
• bsquizz

[myakove]

/add-allowed-user @AmenB

[redhat-qe-bot2]

@AmenB is now allowed to run commands

[AmenB]

/verified

[myakove]

/lgtm
/approve

[rnetser]

@myakove I think that we should get here app (as we do now with new generated resources). wdyt?

[rnetser]

can app and playbook \ image co-exist? iiuc, they cannot. while we do not validate that (the api should); we have opinionated ordering here - i.e if a user passes both, we give app presedence. @myakove wdyt?