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 1859874: Add basic engine URL validation #3948
Bug 1859874: Add basic engine URL validation #3948
Conversation
@gekorob: This pull request references Bugzilla bug 1859874, which is invalid:
Comment 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. |
/bugzilla refresh |
@gekorob: This pull request references Bugzilla bug 1859874, which is invalid:
Comment 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. |
e24af44
to
982f9d4
Compare
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.
Just small comments. LGTM
/retest |
/bugzilla refresh |
@gekorob: This pull request references Bugzilla bug 1859874, which is invalid:
Comment 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. |
982f9d4
to
34c5161
Compare
/lgtm |
Tested locally the change and it works. Waiting the bugzilla label ack to add the approve |
/bugzilla refresh Recalculating validity in case the underlying Bugzilla bug has changed. |
@openshift-bot: This pull request references Bugzilla bug 1859874, which is invalid:
Comment 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. |
/bugzilla refresh |
@gekorob: This pull request references Bugzilla bug 1859874, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 6 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. |
ci/prow/e2e-ovirt — Job succeeded. |
/approve |
/assign @rgolangh |
url, err := url.ParseRequestURI(urlStr) | ||
if err != nil { | ||
return errors.Errorf("The specified URL is invalid, got %s", urlStr) | ||
} | ||
|
||
if url.Scheme != "https" { | ||
return errors.Errorf("The only URL scheme accepted is https, but got %s", url.Scheme) | ||
} |
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.
I think we can replace this with https://godoc.org/github.com/openshift/installer/pkg/validate#URIWithProtocol
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.
also this entire function can be made a little easier to read by early error return.
uri, ok := val.(string)
if !ok {
return invalid expected string
}
return validate.URIWithProtocol(uri, "https")
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.
Hi @abhinavdahiya I didn't know about URIWithProtocol, great suggestion. I've replaced the code.
}{ | ||
{ | ||
url: "engine.example.com", | ||
expectSuccess: false, |
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.
test for actual errors and not just error yes/no
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.
Changed expectations using the Error returned by URIWithProtocol, hope I understood correctly your point.
/retest |
LGTM. Waiting feedback from @abhinavdahiya |
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.
/approve
func validURL(val interface{}) error { | ||
uri, ok := val.(string) | ||
if !ok { | ||
return fmt.Errorf("cannot check url validity on type %v", reflect.TypeOf(val).Name()) |
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.
return fmt.Errorf("cannot check url validity on type %v", reflect.TypeOf(val).Name()) | |
return fmt.Errorf("cannot check url validity on type %T", val) |
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.
Thx for the suggestion.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, dougsland 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 |
/lgtm |
My only comment: If possible, please squash the patches. Otherwise, looks good to me. |
Add a basic URL validator to avoid adding urls with no scheme or scheme different from https Signed-off-by: Roberto Ciatti <rciatti@redhat.com>
c4a16e9
to
3f71c8b
Compare
/test e2e-ovirt |
2 similar comments
/test e2e-ovirt |
/test e2e-ovirt |
/lgtm |
@gekorob: All pull requests linked via external trackers have merged: openshift/installer#3948. Bugzilla bug 1859874 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. |
Add a basic URL validator to avoid adding urls with no
scheme or scheme different from https.
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1859874
Signed-off-by: Roberto Ciatti rciatti@redhat.com