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 support for configuring Hook resources via an AAP template ID (aap), plus deadline, image, playbook, and service_account.
• Behavior Changes
  • Hook output no longer forcibly injects spec.image / spec.playbook; serialization now follows the provided input parameters.
  • image defaulting is conditional: it’s omitted when aap is set, otherwise defaults apply.
• Deprecations
  • A deprecation warning is now always shown when using the legacy Hook interface.

[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]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6f74fef5-d317-4890-a257-e4f23e4c7a65

📥 Commits

Reviewing files that changed from the base of the PR and between 15bbb54 and e346681.

📒 Files selected for processing (1)

• ocp_resources/hook.py

🚧 Files skipped from review as they are similar to previous changes (1)

• ocp_resources/hook.py

---

Walkthrough

Hook is replaced with a deprecated compatibility shim over the generated Hook resource. Its constructor detects omitted images, selects defaults for AAP and standard hooks, and delegates initialization without the former to_dict() override.

Changes

Hook compatibility shim migration

Layer / File(s)	Summary
Deprecated Hook initialization ocp_resources/hook.py	Defines the compatibility shim over the generated Hook, emits a DeprecationWarning, selects None for omitted AAP images or the default image otherwise, and forwards constructor arguments to the generated implementation.

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

Possibly related PRs

• RedHatQE/openshift-python-wrapper#2765: Introduces the generated Hook model wrapped by this compatibility shim.

Suggested reviewers: myakove, rnetser

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title mentions aap_job_template_id, but the PR actually shims Hook to use aap and deprecates the old implementation.	Retitle it to describe the actual change, e.g. a deprecated Hook shim using the generated Hook and aap/jobTemplateId support.
Description check	⚠️ Warning	The description is missing the repository's required headings and describes a different implementation than the one in the PR.	Rewrite it to match the template sections and reflect the shim-based Hook change, including the actual API and migration behavior.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
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 (1) 📘 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 ✓ Resolved 🐞 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

[myakove]

@AmenB

We're landing the schema-aligned generated Hook in a separate PR: #2765 (ocp_resources/_hook_generated.py
— fields: aap, deadline, image, playbook, service_account). That PR does not change hook.py or add deprecation.

Once #2765 (#2765) merges, please retarget this PR to:

1. Keep from ocp_resources.hook import Hook working by turning hook.py into a thin deprecated shim that subclasses / delegates to _hook_generated.Hook.
2. Do not add aap_job_template_id. Use the generated aap: dict instead, e.g. Hook(..., aap={"jobTemplateId": 9}).
3. Keep the legacy default-image behavior in the shim only (same idea as _UNSET here): if image is omitted and aap is set → image=None; if image is omitted and aap
   is not set → apply the current default image + DeprecationWarning.
4. Remove the if/else in to_dict. The generated class already writes only non-None fields (aap / image / playbook / …).

Example shim shape:

  from ocp_resources._hook_generated import Hook as _GeneratedHook                                                                                                  
                                                                                                                                                                    
  _UNSET = object()                                                                                                                                                 
  _DEFAULT_IMAGE = "quay.io/konveyor/hook-runner:latest"                                                                                                            
                                                                                                                                                                    
  class Hook(_GeneratedHook):                                                                                                                                       
      def __init__(self, aap=None, deadline=None, image=_UNSET, playbook=None, service_account=None, **kwargs):                                                     
          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                                                                                                   
          super().__init__(                                                                                                                                         
              aap=aap,                                                                                                                                              
              deadline=deadline,                                                                                                                                    
              image=image,                                                                                                                                          
              playbook=playbook,                                                                                                                                    
              service_account=service_account,                                                                                                                      
              **kwargs,                                                                                                                                             
          )                                                                                                                                                         

MTV call site change: Hook(aap_job_template_id=9) → Hook(aap={"jobTemplateId": 9}).

[AmenB]

/hold

[redhat-qe-bot1]

AmenB is not part of the approver, only approvers can mark pull request with hold

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with 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.

Inline comments:
In `@ocp_resources/hook.py`:
- Around line 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.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4be5a318-eba3-46dc-b17b-f6f49c227353

📥 Commits

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

📒 Files selected for processing (1)

• ocp_resources/hook.py

[rh-bot-1]

Clean rebase detected — no code changes compared to previous head (15bbb54).
The following labels were preserved: commented-coderabbitai[bot], commented-AmenB.