-
Notifications
You must be signed in to change notification settings - Fork 358
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
Adding ssh and url check for ansible repository form #8164
Conversation
At a minimum, let's ressurect the tests for all of the good cases, and probably some made up "bad" cases. Don't think we need every bad case that was presented in the old PR. |
@@ -38,15 +39,10 @@ function createSchema(repositoryId) { | |||
id: 'scm_url', | |||
name: 'scm_url', | |||
isRequired: true, | |||
validate: [{ | |||
type: validatorTypes.REQUIRED, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to keep required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely yes, I don't know of a case when the url wont be required.
d1e7413
to
338a0a9
Compare
Checked commit MelsHyrule@a1ff431 with ruby 2.6.7, rubocop 1.19.1, haml-lint 0.35.0, and yamllint |
LGTM, but I helped. @kavyanekkalapu Please review. @MelsHyrule Can you add some updated screenshots? |
@MelsHyrule Can you also verify in Safari? 😉 |
@Fryguy can confirm works in safari 😆 and updated screenshots |
app/javascript/components/ansible-repository-form/ansible-repository-form.schema.js
Show resolved
Hide resolved
@kavyanekkalapu I am not seeing that bug, when i type |
Ok i will check again |
@MelsHyrule Looks like i didn't have your latest code before. i tested it again and ui looks good to me. |
Fixes ManageIQ/manageiq#21700
Related to ManageIQ/manageiq#21700 and #8157
Before
After