Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the Edit URL modal text for the vSphere provider #774

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

sgratch
Copy link
Collaborator

@sgratch sgratch commented Nov 7, 2023

Reference: #646

Fix the Edit URL modal text for the vSphere provider, based on the create provider help texts:

  1. Rephrase the title and help text field
  2. Support warning text message and icon

Before the fix

Screenshot from 2023-11-07 23-07-43

After the fix

Screenshot from 2023-11-07 23-08-17

Screenshot from 2023-11-07 23-08-34

Screenshot from 2023-11-07 23-08-52

Screenshot from 2023-11-07 23-09-07

Fix the Edit URL modal text for the vSphere provider, based on the create provider help texts:

1. Rephrase the title and help text field
2. Support warning text message and icon

Signed-off-by: Sharon Gratch <[email protected]>
Copy link

sonarqubecloud bot commented Nov 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sgratch sgratch requested review from yaacov and rszwajko November 7, 2023 21:17
@sgratch sgratch added this to the 2.6.0 milestone Nov 7, 2023
@sgratch
Copy link
Collaborator Author

sgratch commented Nov 7, 2023

cc:// @RichardHoch @anarnold97

@yaacov yaacov merged commit 22ade32 into kubev2v:main Nov 8, 2023
5 checks passed
@sgratch sgratch deleted the fix-edit-vsphere-url-help-text branch November 8, 2023 09:34
@RichardHoch
Copy link

@sgratch @yaacov @anarnold97 @ahadas
After much discussion, the description for this field in the documentation was written as "URL of the SDK endpoint of the vCenter on which the source VM is mounted. Ensure that the URL includes the sdk path, usually /sdk. 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."

Do I need to change the description in the documentation?

@yaacov
Copy link
Member

yaacov commented Nov 8, 2023

@RichardHoch hi, thank you for noticing that,
no, me or @sgratch will align the UI to be as the docs in a followup PR

@ahadas
Copy link
Member

ahadas commented Nov 8, 2023

note that with the introduction of "ESXi provider" we'd need to add another description for the case the URL refers to ESXi rather than to vCenter

@sgratch
Copy link
Collaborator Author

sgratch commented Nov 8, 2023

@RichardHoch @yaacov

hi, thank you for noticing that, no, me or @sgratch will align the UI to be as the docs in a followup PR

The current text is aligned with the one appears for the create provider modal.
IIRC, we agreed that the missing part which appears in the doc of "If a certificate for FQDN is specified, the value of this field needs to match the FQDN in the certificate", will not be displayed in UI since it's too detailed information.

Except that, there is nothing to change since we use the same terminology, just shorter.
If you still think we need a followup PR for this field (regardless to ESXi provider), please update here so that the fix won't be dragged to other providers text field changes as well.

@RichardHoch
Copy link

@sgratch Thanks for keeping me honest! :) In that case, everything's fine.

@yaacov
Copy link
Member

yaacov commented Nov 8, 2023

The current text is aligned with the one appears for the create provider modal.
IIRC, we agreed that the missing part which appears in the doc of "If a certificate for FQDN is specified, the value of this field needs to match the FQDN in the certificate", will not be displayed in UI since it's too detailed information.

To clarify:
doc team (@RichardHoch ) decide on the the final text based on dev team ( @sgratch ) base inforamtion.
so in this case, the doc team looked at the original text and decided the final text should be some text, then that the text we should use.

Except that, there is nothing to change since we use the same terminology, just shorter.

when doc team (@RichardHoch ) decide on the text they took this into consideration, as dev team ( me ) we can advise, but the final text is what the doc team decide

@sgratch
Copy link
Collaborator Author

sgratch commented Nov 8, 2023

@yaacov @RichardHoch Sure! No argue about that.
My point is that We had that discussion when we rephrased the create providers URL text, based on the up to date doc. We decided it's ok to leave it as is and this PR for the edit providers is based on that as well.

If we want to rephrase those fields again and add more info or use the exact doc text in all relevant places for all providers, let's decide it now and I'll re-change it again in all places. I understood that the current text is ok back then.

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.

4 participants