Align description of provider properties with the UI #432

RichardHoch · 2023-10-03T09:54:06Z

MTV 2.5.1

Resolves: https://issues.redhat.com/browse/MTV-717 and https://issues.redhat.com/browse/MTV-703 by:

Changing the description of the URL field for vSphere source providers.
Changing the description of the URL field for RHV source providers.
Removing note that said provider names cannot include "."
4-6: Removing an inconsistent "/" in some URLs.

Previews:

https://file.emea.redhat.com/rhoch/251_provider_fixes/html-single/#adding-source-providers [See definitions of "URL" for vSphere and RHV source providers; also see that note about provider names was removed]
https://file.emea.redhat.com/rhoch/251_provider_fixes/html-single/#migrating-virtual-machines-cli_mtv [Step 1, callout 11 and steep 2, callout 2]
https://file.emea.redhat.com/rhoch/251_provider_fixes/html-single/#migrating-virtual-machines-cli_mtv [step 6, callout 2]

RichardHoch · 2023-10-03T10:59:05Z

@bennyz @bkhizgiy @sgratch @yaacov @ahadas Please review this PR. It focuses on the changes we need to make to the documentation for the Create Providers UI (https://access.redhat.com/documentation/en-us/migration_toolkit_for_virtualization/2.5/html-single/installing_and_using_the_migration_toolkit_for_virtualization/index#adding-providers) -- NOT the error messages, only the documentation of the fields. The error messages are in other PRs.

documentation/modules/adding-source-provider.adoc

yaacov · 2023-10-03T11:10:19Z

@RichardHoch thank you for opening this PR 💚

I'll ping here once the text in kubev2v/forklift-console-plugin#744 and kubev2v/forklift-console-plugin#745 are merged so we have a stable point of reference.

RichardHoch · 2023-10-03T11:18:29Z

@ahadas I made the changes you suggested. Please review the PR again.

sgratch

LGTM, except one comment.

documentation/modules/adding-source-provider.adoc

RichardHoch · 2023-10-03T14:37:56Z

@sgratch Made the change you suggested.

yaacov · 2023-10-03T16:06:47Z

@RichardHoch @sgratch the peorpose of this PR is to align with the UI

please wait for kubev2v/forklift-console-plugin#744 and kubev2v/forklift-console-plugin#745.

once @ahadas and @RichardHoch agree on them, I'll merge them and we will copy them here.

RichardHoch · 2023-10-04T15:43:38Z

@yaacov @ahadas I think this PR is ready for QA. Agree? The changes in it fit Sharon's PRs as far as I can tell (error messages are not part of the documentation).

yaacov · 2023-10-05T09:25:08Z

documentation/modules/adding-source-provider.adoc

@@ -100,7 +100,7 @@ ifdef::rhv[]
 * *Provider resource name*: Name of the source provider
 +
 include::snip-provider-names.adoc[]
-* *URL*: API endpoint of the {rhv-short} Manager on which the source VM is mounted
+* *URL*: API endpoint of the {rhv-full} Manager (RHVM) on which the source VM is mounted, for example, `https://rhv-host-example.com/ovirt-engine/api`


in https://github.com/kubev2v/forklift-console-plugin/pull/744/files#diff-075b2af748cc5c8407a5cd156371011523517ef954d89d2bac7d9d5c72d9fe97R29

we chose to write:

URL of the Red Hat Virtualization Manager (RHVM) API endpoint. Ensure the URL includes the "/ovirt-engine/api" path. for example: https://rhv-host-example.com/ovirt-engine/api

I'm ok with current version and ok with uipdateing to match to UI

p.s. not that the for -> For was fixed in the UI in a folowup

@yaacov I added "URL of the..." to all the URL descriptions.

yaacov · 2023-10-05T09:27:57Z

documentation/modules/adding-source-provider.adoc

@@ -78,7 +78,7 @@ ifdef::vmware[]
 . Specify the following fields:

 * *Provider resource name*: Name of the source provider.
-* *URL*: vCenter host name or IP address - if a certificate for FQDN is specified, the value of this field needs to match the FQDN in the certificate.
+* *URL*: SDK endpoint of the vCenter on which the source VM is mounted, for example, `https://vCenter-host-example.com/sdk`. If a certificate for FQDN is specified, the value of this field needs to match the FQDN in the certificate.


In https://github.com/kubev2v/forklift-console-plugin/pull/745/files#diff-78ab8802dcf0222dfbd15eeeac4d1f58cefaec2614e5900cbd2146cd8738a9d8R30

we chose to wirte:

URL of the vCenter SDK endpoint. Ensure the URL includes the "/sdk" path. For example: https://vCenter-host-example.com/sdk

I'm ok with current version and ok with uipdateing to match to UI

p.s.

i see the fqdn comment apear here:
If a certificate for FQDN is specified, the value of this field needs to match the FQDN in the certificate.

@ahadas if it's needed we can add it to the UI, and leave it here ?

p.p.s
from the text in the UI it sounds like it must end with /sdk ... (same with the oVIrt texts)
the URL does not need to end with this post fixes, it's a little misleading, but IMHO we can trust our users to understand this nuance

@yaacov @sgratch "Ensure" means it must be so. If that is not 100% accurate, that is, if it is OK if the URL does not end in /sdk, then you might want do one of the following in the UI:

Remove the sentence.

Change it to "Ensure.../sdk path, if needed."

Change it to "It is recommended that URL includes..." [We try to avoid saying "it is recommended" but if you think it is useful, it is OK]

I'm not sure which of the above is technically correct. You can leave the UI alone, but I'd recommend you change it if the text might confuse users.

thanks @RichardHoch , it did sound off to me.
IMHO "if needed" is best here, we want to ensure the user understand that the URL includes the postifx that can be "/sdk" but can also be other strings like "/east1/sdk/v1" or "/server",

@RichardHoch before this change we used the term "sdk path" to denote any path that lead to the sdk, instead of using "/sdk" that sounds like it must be "/sdk".
Can we say that it needs to include the sdk path, and then say that "/sdk" is the common path used as the sdk path ?

https://github.com/kubev2v/forklift-console-plugin/pull/745/files#diff-78ab8802dcf0222dfbd15eeeac4d1f58cefaec2614e5900cbd2146cd8738a9d8L111

yaacov

LGTM

@RichardHoch hi, not exactly as the UI, but the UI does not need to be verbatim copy of the docs (*)

I commented with the texts chosen for the UI copy of the docs, we can also do a PR for the UI to alight to the texts we end up using here.

cc:// @sgratch

(*) note that the docs is the "source of truth" for the UI, so the UI need to match the docs not the other way around.

RichardHoch · 2023-10-05T11:09:25Z

@ahadas @yaacov Added "URL of the.." to any URL definitions that did not have it. Please review again. I left the "Ensure ..." sentence out of the documentation since it sounded like it was not 100% necessary.

RichardHoch · 2023-10-05T11:12:07Z

@yaacov Our comments overlapped. Would it be better if I add "Ensure..., if needed." to the documentation?

yaacov · 2023-10-05T11:15:13Z

@yaacov Our comments overlapped. Would it be better if I add "Ensure..., if needed." to the documentation?

makes sens to me, @ahadas WDYT ?

p.s. this comment is also for the ovirt engine path, it can be "ovirt-engine/api" and can also be some other pass leading to the ovirt engine api.

yaacov · 2023-10-05T11:57:54Z

@RichardHoch the reason we changed the UI text to include the "please make sure you add the ovirt engine path in oVirt and the sdk path vSphere" was because of users feedback, in your current version, i don't see this emphasis on the need to add the API path.
@ahadas we can drop the "Ensure you add "/skd" "/ovier-engine" " but then, we will return to the original phrasing, the one that users didn't like, because they tried to enter the URL without the path, WDYT ?

yaacov · 2023-10-05T12:02:16Z

@RichardHoch @sgratch I see the discussion moved here :-)
This is good, beacuse the docs is the "source of truth" for the UI.

@sgratch once we agree on text here, please make a PR to copy it the UI.

RichardHoch · 2023-10-05T13:38:21Z

@yaacov @ahadas How's this?

yaacov

LGTM

RichardHoch · 2023-10-05T16:08:33Z

@qiyuann Please review this PR.

documentation/modules/adding-source-provider.adoc

sgratch · 2023-10-05T20:26:29Z

Regarding the FQDN comment appears here and not in the UI:
"If a certificate for FQDN is specified, the value of this field needs to match the FQDN in the certificate."
I intentionally skipped that part in the UI since it seems to me like too much information to put for a field help text. And anyway, we can consider adding this as a help text for the CA certificate field and not for the URL field.
Regarding writing "Ensure the URL includes the xxx path...." for both RHV and vSphere providers URLs:
Another option ,in addition to Richard's suggestions, is to to replace it with something like:
"usually the URL includes the xxx path. For example..."

The "...,if needed" suggested option seems a bit confusing to me, because the user won't know what are the terms for "needed".

Once we will agree on those two issues, I'll make the changes in the UI, if needed :)

documentation/modules/adding-source-provider.adoc

ahadas · 2023-10-08T06:47:40Z

Regarding the FQDN comment appears here and not in the UI:
"If a certificate for FQDN is specified, the value of this field needs to match the FQDN in the certificate."
I intentionally skipped that part in the UI since it seems to me like too much information to put for a field help text. And anyway, we can consider adding this as a help text for the CA certificate field and not for the URL field.

yeah, I don't even think we must add that to the UI - from the cases we get, it looks like insecure connections become more common (but it's good to have it in the documentation and I think the way it is now, is better than moving it to the CA certificate field)

Regarding writing "Ensure the URL includes the xxx path...." for both RHV and vSphere providers URLs:
Another option ,in addition to Richard's suggestions, is to to replace it with something like:
"usually the URL includes the xxx path. For example..."
The "...,if needed" suggested option seems a bit confusing to me, because the user won't know what are the terms for "needed".

I guess you referenced a previous state of this PR as I don't see ... , if needed now

sgratch

LGTM

RichardHoch · 2023-10-08T07:42:28Z

@ahadas @yaacov @sgratch Please review again.

ahadas

minor comment about the paths to align them with the message in the UI
and @RichardHoch we can we also remove the file snip-provider-names.adoc, right?

documentation/modules/adding-source-provider.adoc

yaacov

lgtm

p.s.
you changed the OCP provider text, URL of the endpoint of the API server
we will need to add more text here, but I do not think we need to change the text here, we will go over all the texts in the provider new creation form in Issue kubev2v/forklift-console-plugin#646, @sgratch

sgratch · 2023-10-08T08:20:31Z

p.s. you changed the OCP provider text, URL of the endpoint of the API server we will need to add more text here, but I do not think we need to change the text here, we will go over all the texts in the provider new creation form in Issue kubev2v/forklift-console-plugin#646, @sgratch

It's ok to leave it just to have all URL text msgs start with: "URL of the....".
It will be revised with #646 of course, but for now it's better to leave it as it is now.

Signed-off-by: RichardHoch <[email protected]>

RichardHoch · 2023-10-08T08:47:44Z

@qiyuann This is ready for you.

qiyuann · 2023-10-08T13:09:56Z

LGTM

anarnold97

@RichardHoch - good work

RichardHoch force-pushed the 251_provider_fixes branch from b2f84c8 to 83555fb Compare October 3, 2023 09:55

RichardHoch requested review from ahadas, bennyz, yaacov, sgratch and bkhizgiy October 3, 2023 10:55

ahadas reviewed Oct 3, 2023

View reviewed changes

documentation/modules/adding-source-provider.adoc Outdated Show resolved Hide resolved

documentation/modules/adding-source-provider.adoc Outdated Show resolved Hide resolved

RichardHoch requested a review from ahadas October 3, 2023 11:18

sgratch reviewed Oct 3, 2023

View reviewed changes

documentation/modules/adding-source-provider.adoc Outdated Show resolved Hide resolved

RichardHoch requested a review from sgratch October 3, 2023 14:36

RichardHoch force-pushed the 251_provider_fixes branch from 1b63341 to d323942 Compare October 3, 2023 14:39

yaacov reviewed Oct 5, 2023

View reviewed changes

yaacov approved these changes Oct 5, 2023

View reviewed changes

RichardHoch force-pushed the 251_provider_fixes branch 2 times, most recently from 2ed1b25 to 9d00f28 Compare October 5, 2023 11:05

RichardHoch requested a review from yaacov October 5, 2023 11:34

yaacov approved these changes Oct 5, 2023

View reviewed changes

RichardHoch requested a review from qiyuann October 5, 2023 16:07

sgratch reviewed Oct 5, 2023

View reviewed changes

documentation/modules/adding-source-provider.adoc Outdated Show resolved Hide resolved

ahadas reviewed Oct 8, 2023

View reviewed changes

documentation/modules/adding-source-provider.adoc Outdated Show resolved Hide resolved

documentation/modules/adding-source-provider.adoc Outdated Show resolved Hide resolved

RichardHoch force-pushed the 251_provider_fixes branch from 8682805 to ec61d08 Compare October 8, 2023 07:23

sgratch approved these changes Oct 8, 2023

View reviewed changes

RichardHoch force-pushed the 251_provider_fixes branch from 82d07a1 to 6f88d55 Compare October 8, 2023 07:39

RichardHoch requested review from ahadas, sgratch and yaacov October 8, 2023 07:42

ahadas reviewed Oct 8, 2023

View reviewed changes

documentation/modules/adding-source-provider.adoc Outdated Show resolved Hide resolved

documentation/modules/adding-source-provider.adoc Outdated Show resolved Hide resolved

yaacov approved these changes Oct 8, 2023

View reviewed changes

Align description of provider properties with the UI

97eb324

Signed-off-by: RichardHoch <[email protected]>

RichardHoch force-pushed the 251_provider_fixes branch from d0cd9b8 to 97eb324 Compare October 8, 2023 08:38

RichardHoch requested a review from ahadas October 8, 2023 08:41

ahadas approved these changes Oct 8, 2023

View reviewed changes

ahadas assigned qiyuann Oct 8, 2023

qiyuann approved these changes Oct 8, 2023

View reviewed changes

anarnold97 approved these changes Oct 8, 2023

View reviewed changes

RichardHoch merged commit c903456 into kubev2v:main Oct 8, 2023
1 check passed

Align description of provider properties with the UI #432

Align description of provider properties with the UI #432

Conversation

RichardHoch commented Oct 3, 2023 • edited Loading

RichardHoch commented Oct 3, 2023

yaacov commented Oct 3, 2023

RichardHoch commented Oct 3, 2023

sgratch left a comment

Choose a reason for hiding this comment

RichardHoch commented Oct 3, 2023

yaacov commented Oct 3, 2023

RichardHoch commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaacov left a comment

Choose a reason for hiding this comment

RichardHoch commented Oct 5, 2023

RichardHoch commented Oct 5, 2023 • edited Loading

yaacov commented Oct 5, 2023

yaacov commented Oct 5, 2023

yaacov commented Oct 5, 2023

RichardHoch commented Oct 5, 2023

yaacov left a comment

Choose a reason for hiding this comment

RichardHoch commented Oct 5, 2023

sgratch commented Oct 5, 2023 • edited Loading

ahadas commented Oct 8, 2023

sgratch left a comment

Choose a reason for hiding this comment

RichardHoch commented Oct 8, 2023

ahadas left a comment

Choose a reason for hiding this comment

yaacov left a comment

Choose a reason for hiding this comment

sgratch commented Oct 8, 2023

RichardHoch commented Oct 8, 2023

qiyuann commented Oct 8, 2023

anarnold97 left a comment

Choose a reason for hiding this comment

RichardHoch commented Oct 3, 2023 •

edited

Loading

RichardHoch commented Oct 5, 2023 •

edited

Loading

sgratch commented Oct 5, 2023 •

edited

Loading