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

[MTV-1794] Update provider vCenter validation #1523

Merged
merged 1 commit into from
Mar 20, 2025

Conversation

jpuzz0
Copy link
Contributor

@jpuzz0 jpuzz0 commented Mar 12, 2025

📝 Links

https://issues.redhat.com/browse/MTV-1794

📝 Description

Address feedback related to this comment: https://issues.redhat.com/browse/MTV-1794?focusedId=26727397&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-26727397

🎥 Demo

Screen.Recording.2025-03-12.at.1.29.19.PM.mov

@jpuzz0 jpuzz0 added the bug Categorizes issue or PR as related to a bug. label Mar 12, 2025
@jpuzz0 jpuzz0 requested a review from sgratch March 12, 2025 17:34
@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 13.33333% with 13 lines in your changes missing coverage. Please review.

Project coverage is 36.22%. Comparing base (13484d0) to head (d0d9e67).
Report is 381 commits behind head on main.

Files with missing lines Patch % Lines
.../validators/provider/vsphere/validateVCenterURL.ts 16.66% 3 Missing and 2 partials ⚠️
...ers/modals/EditProviderURL/VSphereEditURLModal.tsx 20.00% 4 Missing ⚠️
...ators/provider/vsphere/vsphereProviderValidator.ts 0.00% 3 Missing ⚠️
...ers/utils/validators/provider/providerValidator.ts 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1523      +/-   ##
==========================================
- Coverage   36.81%   36.22%   -0.59%     
==========================================
  Files         158      157       -1     
  Lines        2548     2595      +47     
  Branches      599      632      +33     
==========================================
+ Hits          938      940       +2     
- Misses       1428     1461      +33     
- Partials      182      194      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@avivtur
Copy link
Collaborator

avivtur commented Mar 16, 2025

/lgtm

Copy link
Collaborator

@sgratch sgratch left a comment

Choose a reason for hiding this comment

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

@jpuzz0 please see comment below
I suggest removing the urlMatchesCertFqdn ui validation and leave only the following validation:

f (isValidIpAddress) {
      return {
        type: 'warning',
        msg: 'The URL is not a fully qualified domain name (FQDN). If the certificate does not match the URL, the connection might fail.',
};

@jpuzz0 jpuzz0 force-pushed the MTV-1794-update-vcenter-validation branch from eff8697 to 2f37dfb Compare March 17, 2025 19:26
@jpuzz0 jpuzz0 requested a review from sgratch March 17, 2025 19:27
@jpuzz0 jpuzz0 force-pushed the MTV-1794-update-vcenter-validation branch from 2f37dfb to 96e1e9e Compare March 17, 2025 19:28
Copy link
Collaborator

@sgratch sgratch left a comment

Choose a reason for hiding this comment

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

@jpuzz0
Thanks for removing the error. Please check the following comments:

  1. Still need to handle the edit URL for vcenter verification since currently when editing a URL of an existing vcenter insecure provider, the warning of "The URL is not a fully qualified domain name (FQDN)..." is initially displayed:
Screencast.from.2025-03-18.16-12-35.webm
  1. Consider rephrasing the warning text to something like:
    "The URL is not a fully qualified domain name (FQDN). If the certificate is not skipped and does not match the URL, the connection might fail. "

@RichardHoch can you please advice?

@sgratch
Copy link
Collaborator

sgratch commented Mar 18, 2025

CC:// @RichardHoch @anarnold97

@jpuzz0
Copy link
Contributor Author

jpuzz0 commented Mar 18, 2025

@jpuzz0 Thanks for removing the error. Please check the following comments:

  1. Still need to handle the edit URL for vcenter verification since currently when editing a URL of an existing vcenter insecure provider, the warning of "The URL is not a fully qualified domain name (FQDN)..." is initially displayed:

Screencast.from.2025-03-18.16-12-35.webm
2. Consider rephrasing the warning text to something like:
"The URL is not a fully qualified domain name (FQDN). If the certificate is not skipped and does not match the URL, the connection might fail. "

@RichardHoch can you please advice?

@sgratch I've updated the text to your suggestion and updated the edit validation.

@jpuzz0 jpuzz0 force-pushed the MTV-1794-update-vcenter-validation branch from 54ba538 to 6b29fb8 Compare March 18, 2025 16:53
@jpuzz0 jpuzz0 requested a review from sgratch March 18, 2025 16:53
Copy link
Collaborator

@sgratch sgratch left a comment

Choose a reason for hiding this comment

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

All issues mentioned above were addresses.
I see that something seems broken with the VSphereEditURLModal url field validation for the vcenter end point. The validation text message is not re-rendered as required and the initial value is also wrong (note that the text message flickers once opening the modal):

Before this fix:

Screencast.from.2025-03-19.12-07-59.webm

After this fix:

Screencast.from.2025-03-19.12-09-20.webm

@jpuzz0
Copy link
Contributor Author

jpuzz0 commented Mar 19, 2025

@sgratch The flickering is due to us now having to fetch the secret to make this validation check - since it depends on whether the connection is marked as secure or not. I will make the request happen on page entry versus modal open to prevent this.

Otherwise, I noticed there was an issue with how I was passing down the input value for URL, so the rest should be fixed here as well.

@jpuzz0 jpuzz0 force-pushed the MTV-1794-update-vcenter-validation branch from 0e9e35a to f7e7e85 Compare March 19, 2025 15:39
@jpuzz0 jpuzz0 force-pushed the MTV-1794-update-vcenter-validation branch from f7e7e85 to d0d9e67 Compare March 19, 2025 15:49
@sgratch sgratch merged commit 2d79ffe into kubev2v:main Mar 20, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants