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: regular expression for validating registry URL #2192

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

ironAiken2
Copy link
Contributor

@ironAiken2 ironAiken2 commented Jan 30, 2024

This PR resolves #2190

Main Idea

There are some chances that user could use customized hostname with port number, but antd input form validation on URL type doesn't work properly on these cases. URL validation need to accept the addresses like "bai-repo:####".

Changes

  1. use new URL for url validation
  2. Set Error Message for Invalid type URL

Checklist: (if applicable)

  • Mention to the original issue
  • Documentation
  • Minium required manager version
  • Specific setting for review (eg., KB link, endpoint or how to setup)
  • Minimum requirements to check during review
  • Test case(s) to demonstrate the difference of before/after

@github-actions github-actions bot added area:ux UI / UX issue. urgency:4 As soon as feasible, implementation is essential. urgency:blocker IT SHOULD BE RESOLVED BEFORE DISTRIBUTING field:UI / UX field:localization labels Jan 30, 2024
@github-actions github-actions bot added the size:M 30~100 LoC label Jan 30, 2024
Copy link

github-actions bot commented Jan 30, 2024

Coverage report for ./react

St.
Category Percentage Covered / Total
🔴 Statements
3.15% (+0.01% 🔼)
101/3209
🔴 Branches
3.34% (+0.02% 🔼)
67/2007
🔴 Functions
1.56% (+0.01% 🔼)
17/1092
🔴 Lines
3.21% (+0.01% 🔼)
101/3150

Test suite run success

20 tests passing in 4 suites.

Report generated by 🧪jest coverage report action from 5b3fc87

@@ -265,6 +262,14 @@ const ContainerRegistryEditorModal: React.FC<
) {
return Promise.reject(t('registry.DescURLStartString'));
}
const pattern = new RegExp(
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason not to use new URL(url) to validate URL?

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry, you mentioned in the PR description. I'll check

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @yomybaby. Besides, the performance of new URL is better than RegEx.

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-2192.d3g9cs6u59b8lw.amplifyapp.com

@@ -265,6 +262,14 @@ const ContainerRegistryEditorModal: React.FC<
) {
return Promise.reject(t('registry.DescURLStartString'));
}
const pattern = new RegExp(

This comment was marked as resolved.

lizable

This comment was marked as resolved.

Copy link
Member

@yomybaby yomybaby left a comment

Choose a reason for hiding this comment

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

LGTM

return Promise.reject(t('registry.DescURLStartString'));
if (value) {
if (
!value.startsWith('http://') &&
Copy link
Member

Choose a reason for hiding this comment

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

You can use protocol property of new URL().

@yomybaby yomybaby merged commit 14c9bb5 into main Feb 1, 2024
8 checks passed
@yomybaby yomybaby deleted the fix/antd-url-validation branch February 1, 2024 08:53
@yomybaby yomybaby changed the title feat: set RegExpression for validating URL pattern fix: regular expression for validating registry URL Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:ux UI / UX issue. field:localization field:UI / UX size:M 30~100 LoC urgency:blocker IT SHOULD BE RESOLVED BEFORE DISTRIBUTING urgency:4 As soon as feasible, implementation is essential.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Antd url type validation doesn't work properly.
3 participants