MTV-1590 Improved hooks description

[RichardHoch]

MTV 2.8

Resolves https://issues.redhat.com/browse/MTV-1590 by revising section 6.4. of the MTV user guide, "https://docs.redhat.com/en/documentation/migration_toolkit_for_virtualization/2.7/html-single/installing_and_using_the_migration_toolkit_for_virtualization/index#adding-hooks-migration-plan-using-api"

Preview: https://file.corp.redhat.com/rhoch/improved_hook_description/html-single/#adding-hooks-mtv-migration-plan

[RichardHoch]

@mnecas @fabiand Please review this PR. Thanks.

[matthewsecaur]

Hi, @RichardHoch ! I made some comments to you directly via Slack. Happy to continue the conversation here, if needed.

[RichardHoch]

@mnecas @matthewsecaur comments:
The text says: "Enter the Hook runner image. The default image is quay.io/konveyor/hook-runner and this value must be used if you are adding an Ansible playbook."
Issue: I don't understand what we mean when we say "this value must be used if you are adding and Ansible playbook". Customers are free to use their own image, are they not? I suppose their custom image could be based on the default hook-runner image, by why do we say is MUST be used? Here is an example of a custom hook-runner image that can be used with a playbook: https://github.com/alezzandro/aap25-ee-vmware-rhel8/tree/main/hook-runner

[mnecas]

Similar as previous comment, the users can provide any image, but if they specify the spec.playbook they need to provide us with image that has an ansible-runner. So they don't need to be based on the hook-runner but only need the ansible-runner.
Additionally we should change that image as we no longer manage the konveyor quay repository and the image is 2 years outdated.
We should not tell the customers to use some upstream image, we should distribute them the image on top of which they can build or let them to build their images and just mention what are the requirements. That is another issue on which we need to jump.

https://github.com/kubev2v/forklift/blob/0429598fe82ff331ab65e72ff391d1cbb3e1c3b4/pkg/controller/plan/hook.go#L225-L235

[RichardHoch]

@mnecas I removed the image we had. What should I write instead of it?

[mnecas]

created for tracking: https://issues.redhat.com/browse/MTV-2024

[RichardHoch]

@mnecas Matthew added some comments in Slack and I copy-pasted them here.

[mnecas]

Similar as previous comment, the users can provide any image, but if they specify the spec.playbook they need to provide us with image that has an ansible-runner. So they don't need to be based on the hook-runner but only need the ansible-runner.
Additionally we should change that image as we no longer manage the konveyor quay repository and the image is 2 years outdated.
We should not tell the customers to use some upstream image, we should distribute them the image on top of which they can build or let them to build their images and just mention what are the requirements. That is another issue on which we need to jump.

https://github.com/kubev2v/forklift/blob/0429598fe82ff331ab65e72ff391d1cbb3e1c3b4/pkg/controller/plan/hook.go#L225-L235

[RichardHoch]

@mnecas @matthewsecaur I made the changes you suggested -- please review this PR again. Thanks.

[matthewsecaur]

Hi, @RichardHoch
Here are some notes:

• In section 6.4.1.2, under the heading "Process:" bullet point 1a, there is this text:

the spec.playbook they need to provide us with image that has an ansible-runner. So they don't need to be based on the hook-runner but only need the ansible-runner.

I think that text is an error and should either be something else or should be removed.

• In section 6.4.2, under the heading "Procedure" bullet points 3b and 4b seem like they should be the same, but they are not. I recommend leaving 3b alone and replacing 4b with same text as 3b.

• Also in section 6.4.2 in the "Example migration hook" there is a typo (I may have inadvertently caused it... sorry!). In the "- add_host" portion of the Ansible playbook, this line:
  name: "{{ workload.vm.ipaddress }}" # ALT "{{ workload.vm.guestnetworks[2].ip }}"
  should be changed to this:
  name: "{{ vm.ipaddress }}" # ALT "{{ vm.guestnetworks[2].ip }}"
  The "workload." used to be there before we changed how to import the YAML files. When we made that change, the variable names also needed to be updated.

• In section 6.4.3 in the first "Note" there appears to be a typo. This text seems like an errant copy/paste:
  "Each hook needs its own Hook needs its own CR" (i.e. the "hook needs its own" is repeated)

• Section 6.4.3 contains the same typo as the "Example migration hook" in section 6.4.2. The "workload." text should be removed from each variable in the "- add_host:" play.

That's all I see for now. Thanks!