Skip to content

feat(v2): MTV Plans role - #81

Open
sabre1041 wants to merge 1 commit into
redhat-cop:v2from
sabre1041:mtv-plans
Open

feat(v2): MTV Plans role#81
sabre1041 wants to merge 1 commit into
redhat-cop:v2from
sabre1041:mtv-plans

Conversation

@sabre1041

Copy link
Copy Markdown
Contributor

Description

New role for managing/creating MTV Plans

Type of Change

  • feat: A new feature
  • fix: A bug fix
  • docs: Documentation changes
  • style: Formatting, missing semi colons, etc; no code change
  • refactor: Refactoring production code
  • test: Adding missing tests, refactoring tests; no production code change
  • chore: Updating configs, etc; no production code change

@sabre1041
sabre1041 deployed to external-ci September 1, 2026 01:05 — with GitHub Actions Active
@sabre1041
sabre1041 changed the base branch from main to v2 September 1, 2026 01:05
@sabre1041
sabre1041 deployed to external-ci September 1, 2026 01:13 — with GitHub Actions Active

@tech2734 tech2734 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice work — this fits perfectly in the pipeline after mtv_provider and mtv_maps. The custom module approach for VM processing is much cleaner than trying to do this in Jinja2, and the unit test coverage is solid.

A few things I noticed:

Bug: Wrong variable name in Process Plan Skeleton

In tasks/main.yml, the plan skeleton task references mtv_migrate_migration_request instead of mtv_plans_migration_request:

__mtv_plans_mtv_plan_skeleton: >-
  {{ lookup('ansible.builtin.template', 'plan_skeleton.yml.j2') |
  from_yaml |
  ansible.builtin.combine({'spec':
  (mtv_migrate_migration_request['plan_overrides'] |
  default({}))}, recursive=true) }}

Looks like a leftover from the v1 mtv_migrate role name.

Short role names for mtv_query_inventory

The include_role calls use the short name mtv_query_inventory instead of the FQCN infra.openshift_virtualization_migration.mtv_query_inventory. Works within the collection but worth aligning for consistency.

No block/rescue error handling

Unlike mtv_provider and mtv_maps, there's no rescue block around plan creation. If the k8s apply or readiness verification fails, there's no diagnostic context gathered (e.g., fetching the Plan CR's status.conditions). Could be useful for troubleshooting failed plans.

Template values not using | to_json

plan_skeleton.yml.j2 outputs values directly without the | to_json filter. The other role templates (mtv_provider, mtv_maps) use | to_json on all Jinja2 values for safe YAML output — worth aligning.

Missing app.kubernetes.io/managed-by label

The Plan template doesn't include the app.kubernetes.io/managed-by: ansible-migration-factory label that's present on Provider, StorageMap, and NetworkMap CRs.

ca_cert not passed to mtv_query_inventory

The k8s_info/k8s tasks correctly pass ca_cert, but the include_role: mtv_query_inventory calls don't forward mtv_query_inventory_openshift_ca_cert_path. Could matter in environments with custom CA chains.

description_lines vs description in docsible headings

Some variables in defaults/main.yml use # description_lines: instead of # description:. Not sure if docsible treats these identically — the other roles all use # description:.

@sabre1041

Copy link
Copy Markdown
Contributor Author

Nice work — this fits perfectly in the pipeline after mtv_provider and mtv_maps. The custom module approach for VM processing is much cleaner than trying to do this in Jinja2, and the unit test coverage is solid.

A few things I noticed:

Bug: Wrong variable name in Process Plan Skeleton

In tasks/main.yml, the plan skeleton task references mtv_migrate_migration_request instead of mtv_plans_migration_request:

__mtv_plans_mtv_plan_skeleton: >-
  {{ lookup('ansible.builtin.template', 'plan_skeleton.yml.j2') |
  from_yaml |
  ansible.builtin.combine({'spec':
  (mtv_migrate_migration_request['plan_overrides'] |
  default({}))}, recursive=true) }}

Looks like a leftover from the v1 mtv_migrate role name.

Short role names for mtv_query_inventory

The include_role calls use the short name mtv_query_inventory instead of the FQCN infra.openshift_virtualization_migration.mtv_query_inventory. Works within the collection but worth aligning for consistency.

No block/rescue error handling

Unlike mtv_provider and mtv_maps, there's no rescue block around plan creation. If the k8s apply or readiness verification fails, there's no diagnostic context gathered (e.g., fetching the Plan CR's status.conditions). Could be useful for troubleshooting failed plans.

Template values not using | to_json

plan_skeleton.yml.j2 outputs values directly without the | to_json filter. The other role templates (mtv_provider, mtv_maps) use | to_json on all Jinja2 values for safe YAML output — worth aligning.

Missing app.kubernetes.io/managed-by label

The Plan template doesn't include the app.kubernetes.io/managed-by: ansible-migration-factory label that's present on Provider, StorageMap, and NetworkMap CRs.

ca_cert not passed to mtv_query_inventory

The k8s_info/k8s tasks correctly pass ca_cert, but the include_role: mtv_query_inventory calls don't forward mtv_query_inventory_openshift_ca_cert_path. Could matter in environments with custom CA chains.

description_lines vs description in docsible headings

Some variables in defaults/main.yml use # description_lines: instead of # description:. Not sure if docsible treats these identically — the other roles all use # description:.

Addressed the key areas of feedback

Signed-off-by: Andrew Block <andy.block@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants