feat: add aap_job_template_id support to Hook class#2759
Conversation
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Cherry-pick Operations
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughHook now accepts an optional ChangesHook AAP job template support
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
PR Summary by QodoAdd AAP/AWX job template support to MTV Hook resource
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
| "playbook": self.playbook, | ||
| }, | ||
| }) | ||
| if self.aap_job_template_id is not None: |
There was a problem hiding this comment.
Why do we have here if;else?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@rnetser thinkg of removing the defult image, WDYT?
There was a problem hiding this comment.
@AmenB for now keep the if else and add a detraction warn about the image default will be gone in next release
There was a problem hiding this comment.
@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?
Code Review by Qodo
1. Positional args API break
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| namespace=None, | ||
| image="quay.io/konveyor/hook-runner:latest", | ||
| playbook=None, | ||
| aap_job_template_id=None, | ||
| client=None, | ||
| teardown=True, | ||
| yaml_file=None, |
There was a problem hiding this comment.
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
| 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, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
2. Unvalidated mixed hook modes 🐞 Bug ☼ Reliability
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.
Agent Prompt
## 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
Add support for AAP (Ansible Automation Platform) hooks. When aap_job_template_id is set, to_dict() produces spec.aap.jobTemplateId instead of spec.image + spec.playbook. The two hook types are mutually exclusive. Also adds type annotations to all __init__ parameters.
AmenB
force-pushed
the
feat/hook-aap-support
branch
from
a19aa01 to
3f9a036
Compare
5 participants
Summary
Add
aap_job_template_idparameter to theHookclass, enabling MTV AAP hook integration — Hook CRs that trigger AWX/AAP job templates during migration instead of running playbooks in containers.What changed
aap_job_template_id=NoneonHook.__init__().to_dict():aap_job_template_idis set → producesspec.aap.jobTemplateId.spec.image+spec.playbook(existing behavior, unchanged).No regression — existing callers that don't pass
aap_job_template_idhit the original code path.Why
The
mtv-api-testsproject (branchfeat/aap-hook-integration) has a new AAP hook integration test that creates a Hook CR viaHook(aap_job_template_id=9). Without this wrapper change, it fails with:Confirmed by running the test against the current PyPI release (11.0.134).
Related Jira tickets
Polarion: MTV-781
Summary by CodeRabbit