-
Notifications
You must be signed in to change notification settings - Fork 605
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
Bug 1826664: handle Builder image detection when url is changed #5145
Bug 1826664: handle Builder image detection when url is changed #5145
Conversation
@debsmita1: This pull request references Bugzilla bug 1826664, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cc @rohitkrai03 |
disableSubmit={!dirty || !_.isEmpty(errors)} | ||
disableSubmit={ | ||
values.git.urlValidation === ValidatedOptions.default || !dirty || !_.isEmpty(errors) | ||
} |
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.
Why do you need to do this when you are adding the validation that will add the error if repo url is not correct? https://github.com/openshift/console/pull/5145/files#diff-ad8be16d16eef75fed4951b2be470be8R234-R244. If the error is there the button will anyways get disabled.
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.
@divyanshiGupta @rohitkrai03 validation will occur only if the user clicks outside the input field. So there can be a scenario where all the required fields are filled and the user goes ahead and edits just the url. The create button is NOT disabled when the user makes changes to the url. As a result, the user can very easily click on the Create button without validating the git url. Below is the gif to give you a clear picture:
I felt that we should be taking care of such cases. Please let me know if you would like to handle this in a different way
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.
Well, we do have a bug that will change the onBlur
behavior to onChange
. That will validate the URL while user is typing so you won't need the explicit check like this. But for now I would say its okay to have this change.
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.
@rohitkrai03 you must be talking about this issue https://issues.redhat.com/browse/ODC-2451, where the validation should take place right away as the user types the url
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.
Right.
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.
The above gif you showed has an inherent problem itself. Validations run every time user clicks on the submit button and we can see that it shows the error after that. So why does the submit handler submits the form even when there is an error?
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.
The submit action was called before the url could be validated and this was possible only because the Submit button was not disabled.
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.
Submit handler waits for the validation to complete. Formik runs the validation again before calling onSubmit handler https://github.com/jaredpalmer/formik/blob/master/packages/formik/src/Formik.tsx#L740. But the problem is git service validations run async and separate from form validation schema and there is no way of checking errors in the onSubmit
handler since formik omits that in formikBag
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.
Yes. This I did mention in the Root cause, in the PR summary, that the url validation that we are doing, if errors occur it doesn't show up in the formik errors.
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.
/lgtm
/test e2e-gcp-console |
2 similar comments
/test e2e-gcp-console |
/test e2e-gcp-console |
/approve cancel The error This change breaks the dev-console from using private git repos because those URLs are always unreachable. |
Looks like I missed private repository use case. Sorry for that. Should we change the error to be something more generic which also conveys that your repo might be private and you should try to add a secret for it? But even with the secret I don't think the validation status will change since git service can't make use of that secret to validate the repo. So either we make changes to git service which then validates the URL with newly added secret or we do nothing and let user proceed without disabling the submit button. |
We can at least show more informative error (or alert) message saying that the repo is not reachable as it can be either invalid or a private one. |
I think I've seen another bug to change the wordings of the error but the changes in your PR will make private repos unusable with git import. So either we look at another solution similar to what I mentioned in my earlier comment or we just don't do this change at all. |
/approve cancel |
/hold |
37f13e6
to
b82ec36
Compare
b82ec36
to
97ab3cf
Compare
@debsmita1: This pull request references Bugzilla bug 1826664, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/hold cancel |
/cc @christianvogt |
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.
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt, debsmita1, rohitkrai03 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
@debsmita1: All pull requests linked via external trackers have merged: openshift/console#5145. Bugzilla bug 1826664 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Fixes
https://issues.redhat.com/browse/ODC-3159
Analysis / Root cause
When the URL is unreachable, we don't have the capability to decide whether it's invalid or a private one because in both cases it will show the same error.
Solution Description
In this bug fix, I am trying to ensure that the builder image is reset if the user never manually touched the field, in that case, the form will wait for validation of the URL and auto/manual selection of builder image.
GIF
/kind bug